Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The distributions transforms name is misleading #258

Open
lucianopaz opened this issue May 28, 2020 · 2 comments
Open

The distributions transforms name is misleading #258

lucianopaz opened this issue May 28, 2020 · 2 comments

Comments

@lucianopaz
Copy link
Contributor

Background

Following some discussions that took place on our discourse channel, over at pymc3 and in particular in #246, it came to our attention that the transform argument of a distribution is misleading. Some users assume that this will work like tfd.TransformedDistribution, where some operation is applied to a base distribution and a resulting transformed distribution emerges. However, what we have been calling a distribution's transform doesn't do this.

  • First of all, it is only used while performing inference. This means that our transform is never called when drawing samples from the prior or from the posterior predictive.
  • Second, our transforms work backwards from what users expect. The goal they serve is to help MCMC proposal step functions. They do this by allowing the step functions to propose candidate values in MCMC anywhere on the real line (potentially in R^n for multidimensional distributions). Then, the inverse of the transform assigned to the distribution is applied on the proposed value in order to map the proposal onto the distribution's support. For example, in the HalfNormal distribution, the Log transform is used. The distribution's support is only for strictly positive values, this means that a MCMC proposals that were negative wouldn't be supported by the distribution's log_prob, and could raise errors, or even worse, undefined behavior. What the Log transform does, is it creates a new variable called __log_{rv_name} that is used for proposals by the MCMC transition kernel. The proposed values are then transformed using the Log's inverse (the exponential function) in order to map the proposals onto the distribution's support (the positive values).

What should we do?

We should change the name of the transform argument to something like uncontraining_op, which doesn't lead to the ambiguity with tfd.TransformedDistribution. We should also rename the pymc4/distributions/transforms.py module to something in line with the operation name we choose. Finally, we should also rename the transformed_values and untransformed_values in the SamplingState to something that matches the operation name we choose to replace transform with.

@Sayam753
Copy link
Member

Hi @lucianopaz
I also find naming convention for Sigmoid transform to be misleading here.

  1. For mapping to positive values, it makes sense to use Exp bijector's forward function with name of inverse as it is under Log transform.
  2. But for Sigmoid, is it a good idea to run forward function with the name of inverse or should the JacobianPreference=Forward be used?

@lucianopaz
Copy link
Contributor Author

@Sayam753, you're right. Due to our confusing usage of the operation, we should have named it LogOdds instead! We can open that in another issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants