pymc icon indicating copy to clipboard operation
pymc copied to clipboard

Rename `forward` and `backward` methods of the `Transform` class

Open jessegrabowski opened this issue 7 months ago • 3 comments

Description

The Transform class is one of the most confusing parts of PyMC, and issues using them are frequent on the discourse. See here and here for recent examples.

When I go in to help people with these issues, I am tasked with explaining what is going on, which involves reasoning about what transformers do. Consider this code from init_point.py:

        transform = rvs_to_transforms.get(variable, None)

        if transform is not None:
            value = transform.forward(value, *variable.owner.inputs)

        if variable in jitter_rvs:
            jitter = pt.random.uniform(-1, 1, size=value.shape)
            jitter.name = f"{variable.name}_jitter"
            value = value + jitter

        value = value.astype(variable.dtype)
        initial_values_transformed.append(value)

        if transform is not None:
            value = transform.backward(value, *variable.owner.inputs)

        initial_values.append(value)

The transform goes "forward" (from where?), applies jitter, then goes "backward" (to where?). In my opinion, the names of these methods do not help the reader understand the role of the Transform. While this certainly isn't what makes Transformers confusing to the end user, it certainly doesn't help.

My proposal would be to rename forward to unconstrain and backward to constrain. These names aren't perfect, I have at least two objections:

  1. "unconstrain" isn't a word. to_unconstrained and to_constrained would be more correct, but they are uglier imo.
  2. To me these imply that the operations are idempotent, but they are not.

Despite these flaws, reading the code with these names I can at least understand that the incoming samples are expected to be constrained (thus the application of the unconstrain method). The jittering is happening in the unconstrained space (that makes sense) then the samples are sent back to the constrained space at the end.

Again, none of this will make users understand that forward sampling doesn't use the transforms, or that initial points need to be manually specified under transformations! But I still think it's a justified change.

Under this proposal, the forward and backward methods can stay as aliases to constrain and unconstrain with a DeprecationWarning

jessegrabowski avatar Jul 28 '24 04:07 jessegrabowski