pymc-devs / pymc

Bayesian Modeling and Probabilistic Programming in Python
https://docs.pymc.io/
Other
8.72k stars 2.02k forks source link

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

Open jessegrabowski opened 3 months ago

jessegrabowski commented 3 months ago

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

BarryByte commented 3 months ago

@jessegrabowski Assign me this issue, I can help with this.

jessegrabowski commented 3 months ago

You can definitely take it, but I want to wait for input from other devs before doing anything.

ricardoV94 commented 3 months ago

I much prefer the new suggested names. I use those terms all the time myself