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

Implement default_transform and transform argument for distributions #5674

Closed
Tracked by #7053
ricardoV94 opened this issue Mar 31, 2022 Discussed in #5404 · 0 comments · Fixed by #7207
Closed
Tracked by #7053

Implement default_transform and transform argument for distributions #5674

ricardoV94 opened this issue Mar 31, 2022 Discussed in #5404 · 0 comments · Fixed by #7207

Comments

@ricardoV94
Copy link
Member

ricardoV94 commented Mar 31, 2022

Discussed in #5404

Most of the times when users apply a custom transform, they do it because they want to "distort" the base distribution (i.e., to impose ordering, sum to a constant, centered parametrization). This however, overrides default transforms that are set for constrained distributions, leading to inefficient sampling.

The correct thing to do in this case would be the Chain the default transform with the new one, so that the "distorted" distribution is still stable during sampling. However, this is pretty verbose and most users are not even aware of the default transform behavior.

The proposed solution is to implement a new transform keyword default_transform, that behaves like transform now does. The transform keyword, if set, would then be chained together with whatever the default_transform is (or used as is when a distribution does not have a default transform).

If users want to disable any transform whatsoever, they will now have to set default_transform to None. This is usually only done for pedagogical reasons. We can detect if users are trying to do this the old way, when they set transform=None (the default is a flag variable UNSET), and raise an error that explains the changed behavior. Setting transform=None would never have an effect because that is always the default behavior.


In addition we can use this information raise a warning when users do prior predictive, which are generally invalid for "distorted" distributions. This is very similar to what we already do for models containing Potentials for the very same reasons

pymc/pymc/sampling.py

Lines 2014 to 2020 in 80b5e86

if model.potentials:
warnings.warn(
"The effect of Potentials on other parameters is ignored during prior predictive sampling. "
"This is likely to lead to invalid or biased predictive samples.",
UserWarning,
stacklevel=2,
)

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

Successfully merging a pull request may close this issue.

1 participant