Skip to content

Fix enable_validation#2201

Open
renecotyfanboy wants to merge 4 commits into
pyro-ppl:masterfrom
renecotyfanboy:master
Open

Fix enable_validation#2201
renecotyfanboy wants to merge 4 commits into
pyro-ppl:masterfrom
renecotyfanboy:master

Conversation

@renecotyfanboy
Copy link
Copy Markdown
Contributor

Hi there,
Here is a super small fix for validation. Currently, the validation context is handled using _VALIDATION_ENABLED global variable. By default _VALIDATION_ENABLED=True and _validate_args=False by default, which trigger weird behaviors when enabling / disabling the validation through contexts. But since everything is handled through mutating _VALIDATION_ENABLED, it sometimes produces weird behaviors.

The simplest repro is :

print(dist.Uniform(0, 1).log_prob(-0.5)) # -0.0 (expected)

with numpyro.validation_enabled(True):
    print(dist.Uniform(0, 1).log_prob(-0.5)) # -inf (expected)

print(dist.Uniform(0, 1).log_prob(-0.5)) # -inf (expect -0.0)

The current fix produces the expected behavior.

@juanitorduz
Copy link
Copy Markdown
Collaborator

juanitorduz commented May 26, 2026

@fehiepsi : shall we keep _VALIDATION_ENABLED and _validate_args equal to True?

@fehiepsi
Copy link
Copy Markdown
Member

We decided to enable validation by default. I think setting the attribute to the global one might fix the issue.

Copy link
Copy Markdown
Collaborator

@juanitorduz juanitorduz left a comment

Choose a reason for hiding this comment

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

Minor point, right @Qazalbash ?

Comment thread numpyro/distributions/distribution.py Outdated
from . import constraints

_VALIDATION_ENABLED = True
_VALIDATION_ENABLED = False
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Based on the comment above, we should keep _VALIDATION_ENABLED = True

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is fixed. Note that this changes the behavior by default and might be a breaking change for some codebases. However, I find that it is much better to have validation enabled by default, as it was quite unintuitive in the first place.

Copy link
Copy Markdown
Collaborator

@Qazalbash Qazalbash left a comment

Choose a reason for hiding this comment

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

@juanitorduz Yes, the global variable value should be reverted.

Copy link
Copy Markdown
Collaborator

@Qazalbash Qazalbash left a comment

Choose a reason for hiding this comment

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

@Qazalbash
Copy link
Copy Markdown
Collaborator

I think CI will trigger after @juanitorduz approves. Let's merge this PR only when the latest CI passes.

@juanitorduz
Copy link
Copy Markdown
Collaborator

@renecotyfanboy thanks! Can you push an empty commit (e.g., git commit -m"empty" --allow-empty ) to try to trigger the CI? 🙏

@renecotyfanboy
Copy link
Copy Markdown
Contributor Author

Enabling validation by default broke some tests in the CI. Should I simply wrap them around with numpyro.validation_enabled(False) ? I am not sure if I am willing to fix them properly 😆

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.

4 participants