-
-
Notifications
You must be signed in to change notification settings - Fork 426
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
extend model arg usage in io_pymc3 to fix plot_ppc with prior #1045
Conversation
@@ -121,8 +121,7 @@ def arbitrary_element(dct: Dict[Any, np.ndarray]) -> np.ndarray: | |||
def find_observations(self) -> Optional[Dict[str, Var]]: | |||
"""If there are observations available, return them as a dictionary.""" | |||
has_observations = False | |||
if self.trace is not None: |
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.
I don't really like leaving out the check that tells the caller why they are not getting observations out of their trace when they expect it. Perhaps replace my error with a UserWarning
?
It seems like Arviz tries to get whatever information it can out of a trace, and ignores whatever it can't figure out. This makes it relatively robust, but means that the user doesn't know that they could get more information into the InferenceData
by providing a model.
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.
I completely agree, I was just waiting in order to replace it with an informative warning or a deprecation warning.
Per my comment in the code and your question in the MR description -- I agree it would be a good idea to require supply of the model. I think this is particularly important because the user could get an inappropriate model out of the trace. |
Only the |
I decided to start with the deprecation on no model nor trace (it will be triggered in cases such as only prior or only posterior predictive/predictions). We can make it more strict next release by requiring a model even when the trace is present and eventually remove the I think it is ready to merge |
I agree. If the tests pass again, we should merge. Looks very good! |
This allows to make prior predictive checks right away with PyMC3!
I am not really sure about the way to advertise this though, is there a PyMC3 tutorial on prior predictive sampling? @canyon289 @aloctavodia |
This is great -- thanks guys!!
In general, I noticed on the Discourse that people find it hard to understand the concept of prior/posterior pred checks -- or just don't know about them. So I think an intro tutorial would be really useful. |
Description
Try to improve handling of prior groups and observed data in
io_pymc3
. Related to #1002. It introduces an important change in behaviour, it proposes to exclude theobserved_data
group whenever predictions are present. @corriebar @rpgoldmanI also extended tests on
io_pymc
and improved the helper functioncheck_multiple_attrs
.And finally,
how do you feel about deprecating the use ofFor now not using a model nor trace will raise a pending deprecation warning as a start. @canyon289 @aloctavodia @ColCarrollfrom_pymc3
without model nor trace (or even without model). I was thinking on adding a warning.Related to #939.
Checklist
PR format?