Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #299 +/- ##
==========================================
- Coverage 84.40% 83.21% -1.20%
==========================================
Files 42 42
Lines 5702 5786 +84
==========================================
+ Hits 4813 4815 +2
- Misses 889 971 +82 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
aloctavodia
force-pushed
the
diagnose
branch
from
199cc3a to
268eac0
Compare
aloctavodia
force-pushed
the
diagnose
branch
from
268eac0 to
2c45f62
Compare
|
Nice work @aloctavodia, I think the overall structure is good! The message tracking approach is great for PPLs like numpyro that wont have the same sample stats available by default. The one callout I have is that I think ESS and rhat calculations will get expensive on larger deterministic sites. Is it worth excluding them or having a flag that excludes them by default? At the very least, filter_vars covers this (I think) |
|
|
||
| # Check tree depth | ||
| if sample_stats_available and "tree_depth" in sample_stats: | ||
| reached_max_treedepth = sample_stats["reached_max_treedepth"] |
There was a problem hiding this comment.
I know you mentioned this isnt ready for review yet, but I noticed this bug when trying to skim through at a high level so figured I might as well call it out
its checking for a tree_depth key but then selecting reached_max_treedepth
There was a problem hiding this comment.
Good catch, in the first version I followed cmdstan too closely and I used tree_depth.
There was a problem hiding this comment.
This is ready for review, if you have more comments. I was just waiting for feedback before adding tests.
There was a problem hiding this comment.
We should add reached_max_treedepth to https://python.arviz.org/en/stable/schema/schema.html#sample-stats. It is not there so I would assume it is PyMC only
| ess_min_ratio=0.001, | ||
| bfmi_threshold=0.3, | ||
| show_diagnostics=True, | ||
| return_diagnostics=False, |
There was a problem hiding this comment.
maybe we could take a page from the plots side here and allow a stats dictionary argument. That being said, here I would only allow providing pre-computed elements to keep things simple. For example:
rhat = az.rhat_nested(idata, superchains=...)
az.diagnose(idata, stats={"rhat": rhat})
In that particular case it would make it easier to take advantage of diagnose even if wanting to use rhat_nested instead of regular rhat. They could also change the probability at which to compute ess_tail
| Minimum acceptable ratio of ESS to total samples. Parameters with | ||
| ESS/N < ess_min_ratio will be flagged. | ||
| A flag is also emitted if ESS is lower than 100 * number of chains. | ||
| bfmi_threshold : float, default 0.3 |
There was a problem hiding this comment.
we have this as an rcParam I think now
Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>
Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>
4 participants
This follows the
diagnosefunction from CmdStan. A couple of differences are :"reached_max_treedepth"fromsample_stats. Not sure if this is available for all PPLs.Before adding tests, I would like to get feedback about the overall behaviour of this function.