-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Feature/transform aware bounds checking #8046
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
base: main
Are you sure you want to change the base?
Feature/transform aware bounds checking #8046
Conversation
|
|
pymc/logprob/utils.py
Outdated
| ): | ||
| super().__init__(ParameterValueError, msg) | ||
| self.can_be_replaced_by_ninf = can_be_replaced_by_ninf | ||
| self.rv = rv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.rv is not a good strategy. We don't want state at the Op level like this, if it's needed it should be in the graph as an extra input.
Example if we do model.clone() it will now reference the old RV, not the cloned one
pymc/pytensorf.py
Outdated
| rvs_to_transforms = {} | ||
|
|
||
| # Use transform-aware rewrite if we have transforms and check_bounds is enabled | ||
| if check_bounds and rvs_to_transforms: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic on transforms / bounds should be moved away from pytensorf.compile and model.logp or model.fn (#8038)
43a31b3 to
e707627
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ricardoV94,
Implemented transform-aware bounds checking (#8039) with your #8038 feedback:
- ✅ Moved check_bounds logic from
pytensorf.compile()toModel.compile_fn() - ✅ rv passed as graph input (compatible with
model.copy()) - ✅ Added create_transform_aware_check_rewrite() using
rvs_to_transforms
Distribution logp methods are staticmethods without RV access. What's the preferred approach to pass RV context?
Thanks!
ff64546 to
67bc3f6
Compare
|
@ricardoV94, Can you please review my changes and tell me the next steps? I |
|
@Utkarshkarki I don't have capacity to guide in this PR, as it is rather technical. I suggest you try another issue if it's not obvious for you how to proceed alone (it is not obvious for me either until I think a bit more about it). An easier starting point is to add rewrites like |
|
i will look into it |

This PR makes bounds checking in PyMC transform-aware, allowing redundant runtime checks
(
CheckParameterValue → Switch(condition, logp, -inf)) to be safely skipped when the associatedconstraint is already enforced by a variable transform (e.g.,
HalfNormalwithLogTransform).The change introduces optional metadata on
CheckParameterValueand a transform-aware rewriteused during
compile(). Checks are removed per parameter, never globally, and only when atransform provably guarantees the constraint. Existing behavior is preserved when metadata
is absent.
Distribution
logpimplementations are intentionally left unchanged; guidance is requested onthe preferred way to attach RV context in a follow-up.
Related Issue
model.check_boundsin favor of graph introspection #8039Checklist
Type of change