[WIP] Add group argument in plot_ppc (fix #1002) #1008
Conversation
| @@ -38,34 +38,34 @@ def plot_ppc( | |||
| backend=None, | |||
| backend_kwargs=None, | |||
| show=None, | |||
There was a problem hiding this comment.
group must be added here somewhere.
There was a problem hiding this comment.
Ohh, what is the reason for that?
Should I add it below backend_kwargs?
arviz/plots/ppcplot.py
Outdated
| @@ -110,6 +110,9 @@ def plot_ppc( | |||
| check the plotting method of the backend. | |||
| show : bool, optional | |||
| Call backend show function. | |||
| group : str, optional | |||
There was a problem hiding this comment.
I would use group : {"prior", "posterior"}, optional trying to follow numpydoc style. Also, just to be extra clear, I would only allow two options, prior or posterior.
There was a problem hiding this comment.
Yeah that makes sense
arviz/plots/ppcplot.py
Outdated
| if group not in ("posterior_predictive", "prior_predictive", "observed_data"): | ||
| raise TypeError( | ||
| '`group` argument must be `posterior_predictive`, `prior_predictive` or `observed_data`' | ||
| ) |
There was a problem hiding this comment.
I would divide the argument checks in 2 parts. First check that the group is one of the valid options, and then make sure that the 2 needed groups are present in the inference data object (like it was done before with the hasattr). Also, note that if group="posterior" then the inference data must have posterior predictive and observed data groups but prior predictive is not needed
There was a problem hiding this comment.
Also, make sure to not use group as argument and loop variable at the same time ;)
There was a problem hiding this comment.
Yeah actually I was confused earlier about how to check for the group and display error, now I got it. Thanks :)
arviz/plots/ppcplot.py
Outdated
| raise TypeError( | ||
| '`data` argument must have the group "{group}" for ppcplot'.format(group=group) | ||
| ) | ||
| if group == "posterior": |
There was a problem hiding this comment.
We should start by checking group in ("prior", "posterior"): and raise an error if it is not.
This would also allow to simplify the attribute checking by using for groups in ("{}_predictive".format(group), "observed_data"):...
There was a problem hiding this comment.
Yeah and it looks much cleaner. Thanks!
nitishp25
force-pushed
the
add-prior-in-plot-ppc
branch
from
038bfca to
b6d6fbd
Compare
|
LGTM |
|
Was there a specific reason for moving the |
|
In the first commits group was only in the docstring, it was not an argument of the function |
|
I just checked the first commit again and it's the last argument there (line 41), though it doen't matter now :) |
|
I must have missed it then, sorry. |
|
Thanks for the PR! |
|
No problem :) |
Add
groupargument inplot_ppcinppcplot.pyUpdate bokeh backend of
plot_ppcUpdate matplotlib backend of
plot_ppcfixes #1002