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

Fix dependence of Uniform logp on bound method #4541

Merged
merged 6 commits into from
Mar 16, 2021

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Mar 14, 2021

This PR fixes #4499

The Uniform and Discrete Uniform logp methods depended on the dist_math::bound method for correct evaluation of multiple values. This is problematic because the bound can be disabled by setting check_bounds = False in pm.Model().

Other changes:

  1. I tried to improve the documentation of the bound method to emphasize this. I also tried to make it a bit more clear what the "broadcast_conditions" secret keyword argument means and when it should be used. I would suggest this to be made an optional keyword argument in the function call of bound, and perhaps even rename it to multivariate=False to make it more obvious when should it be used. Let me know if you agree and I can add it to this PR as well.

  2. I tweaked the docstring of check_bounds to emphasize this should not be used for models containing discrete variables as some of these depend on bound checks when sampled (unless they have specific samplers that cannot propose invalid values or the logp happens to behave nicely when given out-of-bound points)

  3. Removed rendudant tt.all() calls in the bound conditions of several multivariate distributions.


Do we need a release note?

Depending on what your PR does, here are a few things you might want to address in the description:

@twiecki
Copy link
Member

twiecki commented Mar 16, 2021

LGTM - thanks! Still needs a line in the release notes.

- Logp method of `Uniform` and `DiscreteUniform` no longer depends on `pymc3.distributions.dist_math.bound` for proper evaluation (see [#4541](https://github.com/pymc-devs/pymc3/pull/4541)).
- ...

## PyMC3 3.11.2 (14 March 2021)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't include the entire section: Some changes in v3 (the stats and plotting functions) are not included on the master branch. See #4528.

Instead, we could just mark the "also-in-v3-changes" with "(released in 3.11.2)"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarcoGorelli do you know of best practices when it comes to changelog files in a world with multiple branches and backporting?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. What about just repeating again this line from V3.11.0?

  • ⚠ Many plotting and diagnostic functions that were just aliasing ArviZ functions were removed (see 4397). This includes pm.summary, pm.traceplot, pm.ess and many more!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the only bump between V3.11.1 -> V3.11.2 -> V4.0.0 no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now we could do that. I'll make sure to do the PR such that pm.plots and pm.stats are re-introduced on master too.
Then we can edit the release notes again..

@michaelosthege michaelosthege merged commit b2e64cb into pymc-devs:master Mar 16, 2021
sthagen added a commit to sthagen/pymc-devs-pymc that referenced this pull request Mar 17, 2021
Fix dependence of Uniform logp on bound method (pymc-devs#4541)
@ricardoV94 ricardoV94 deleted the fix_check_bound_logp branch April 22, 2021 07:27
ricardoV94 added a commit to ricardoV94/pymc that referenced this pull request May 11, 2021
ricardoV94 added a commit to ricardoV94/pymc that referenced this pull request May 12, 2021
brandonwillard pushed a commit that referenced this pull request May 13, 2021
twiecki pushed a commit that referenced this pull request Jun 5, 2021
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

Successfully merging this pull request may close these issues.

ValueError when using check_bounds=False in a model with DiscreteUniform
3 participants