-
Notifications
You must be signed in to change notification settings - Fork 31
Add get_merged_chains function #409
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
Add get_merged_chains function #409
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.
Thanks for the PR @ChristianMichelsen!
I think we should make it simpler to work with the output of generated_quantities
if possible. Unfortunately, I think there are some issues with this PR.
Most importantly, DynamicPPL does not depend on MCMCChains but only AbstractMCMC and hence MCMCChains-specific functionality does not seem to belong to DynamicPPL.
Regarding the implementation some high-level suggestions are:
- Reduce the number of functions as it seems many functions are only called in one place
- Avoid names such as
get_...
- Avoid passing around
Dict
s but instead use multiple function arguments whenever possible (also reducing the number of functions will help here) - Add tests and make tests in docstrings doctests by using
jldoctest
instead ofjulia
Maybe an alternative approach here could be to add conversion methods convert(::Type{<:MCMCChains.Chains}, ::Matrix{<:NamedTuple})
etc. to MCMCChains. Then it would be easy to obtain a Chains
object if desired (it's not always desired, the generated quantities don't even have to be arrays or scalars but could be of some other quite arbitrary type). MCMCChains also already supports concatenation of chains, so it would be also possible to concatenate the resulting chain with the original chain if desired.
Thanks for your reply, @devmotion! It seems like my code should maybe be taken somewhere else. Do you have any suggestions? Turing.jl? MCMCChains.jl? Regarding your suggestions, I have a couple of comments:
My updated code is below, now as a single module:
|
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #409 +/- ##
==========================================
- Coverage 76.40% 75.24% -1.17%
==========================================
Files 21 22 +1
Lines 2522 2561 +39
==========================================
Hits 1927 1927
- Misses 595 634 +39
☔ View full report in Codecov by Sentry. |
Closed in favour of #594 |
I have tried to add some small helper functions that allows one to merge chains of a model with the generated quantities.
For more information regarding the background of this, please see this Discourse thread.
I am very new to the internals of DynamicPPL.jl, so this is most likely not the optimal implementation, but I was encouraged on Slack to make this PR anyways.
Hope this can help in some way.