-
Notifications
You must be signed in to change notification settings - Fork 2
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
Issue 63: Implementing SBC #68
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.
- Thank you very much for this PR @SamuelBrand1 !
- I have left some initial comments.
- I would like to critically go through some more of the code in
sbc_model_checking.qmd
and the tests. - Most of the comments are lower priority but nice to have.
- Rank-uniformity checks + variables constitute the only moderately impactful suggestion, in my mind.
- The notebook is quite nice (as mentioned in private communication).
- I will try to respond promptly to an return remarks you have to mine.
- Some of my comments can be resolved after reading, others can be resolved once you've stated your preference and you believe I've read the comment.
Co-authored-by: upx3 (CFA) <127630341+AFg6K7h4fhy2@users.noreply.github.com>
Co-authored-by: upx3 (CFA) <127630341+AFg6K7h4fhy2@users.noreply.github.com>
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.
A few minor comments/questions, but this is looking very good! Thanks, @SamuelBrand1!
Thanks! All of your comments are leftovers from me grabbing a pymc/bambi implementation https://github.com/arviz-devs/simuk . I've put an issue up over there arviz-devs/simuk#19 so in the future we might just be importing their package but I hope this implementation works for now. Note that: a quick implementation in julia I did https://github.com/SamuelBrand1/SBC returns the p-value of the ChiSq uniformity test on the rank statistics. This could be a handy issue/next-step to go along with the viz. |
Replaced with numpyro.distributions.DiscreteUniform
Following @dylanhmorris 's comments:
The new method for the SBC class has a new unit test. |
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.
- Thank you @dylanhmorris for your comments on this PR!
- Thank you again @SamuelBrand1 for this work!
- The tests and notebook both run successfully for me locally.
- There is nothing else within scope that I believe needs to be added for this PR to be merged.
- I approve this PR.
This is an ongoing PR for implementing SBC.This PR is ready for review now. It implements SBC methods very closely modelled on https://github.com/arviz-devs/simulation_based_calibration but for
numpyro
models.For more extensive explanation see the committed notebook which covers the theoretical basis for SBC as well as an example implementation.
NB: the problem with pre-commit which #67 was aimed at is obviously not resolved.This PR closes #63