-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
UserWarning if doing predictive sampling with models containing Potentials #4419
Conversation
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.
Looks good! A simple test would be great.
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.
Nice, thanks @ricardoV94 ! I agree with @michaelosthege that a simple test would be nice.
Also, this should probably be added to fast_sample_posterior_predictive
too: https://github.com/pymc-devs/pymc3/blob/1769258e459e8f40aa8a56e0ac911aa99e7f67de/pymc3/distributions/posterior_predictive.py#L156
Nice catch! I added some tests, do you think they are an overkill? |
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.
The tests are overkill in the sense that they will add unnecessarily lots of runtime.
Instead of calling pm.sample()
which will be a complete overkill in terms of compute, it would be better to use az.InferenceData.from_dict
and manually create a "trace" using numpy.random
.
Agreed! By any chance, do you know if there is any test already doing something like this? |
It doesn't looks like that's the case so far. Many tests probably could. You can inspect the existing trace to find out which variables you'll need. Something along the lines of
|
I guess you can also take some cues from ArviZ test suite -- we do stuff like that IIRC, and we also have built-in datasets |
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.
All good, thanks @ricardoV94 🍾
BTW @ricardoV94, do you have an email address I can contact you on please? |
As discussed in #3865
Do we want tests for the Warning?