pymc
pymc copied to clipboard
Rename `forward` and `backward` methods of the `Transform` class
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 Transformer
s 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:
- "unconstrain" isn't a word.
to_unconstrained
andto_constrained
would be more correct, but they are uglier imo. - 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