Skip to content
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

Added extension for MCMCChains #514

Merged
merged 7 commits into from
Aug 8, 2023
Merged

Conversation

torfjelde
Copy link
Member

@torfjelde torfjelde commented Aug 7, 2023

There is some functionality in DPPL that dispatches on chain::AbstractChains while we are in fact assuming the behavior of chain::MCMCChains.Chains.

This PR adds an extension for MCMCChains.jl so that we can specialize further when it makes sense.

In particular, in the case of generated_quantities(model, chain) we warn the user if some variables are present in chain but not in model. This makes sense if all the variables present in chain are parameters, but less so when we are also working with internal parameters; in such a scenario, the user end up seeing tons of warnings regarding unused variables (which are in fact internal variables of the sampler). In this scenario, it makes sense to specialize further on MCMCChains.Chains and call get_sections(chain, :parameters) before calling setval_and_resample!; this we can now do in the extension.

Note that this I'm not adding Requires.jl or a new dependency in this PR; the change mentioned above is just something that users on Julia 1.9 gets for free without any cost to the ones on Julia 1.8. We can discuss whether we should also add Requires.jl, etc. but I think for now this is an okay thing to do 🤷

EDIT: Other methods that should eventually be moved is

ext/DynamicPPLMCMCChainsExt.jl Outdated Show resolved Hide resolved
test/ext/DynamicPPLMCMCChainsExt.jl Outdated Show resolved Hide resolved
torfjelde and others added 2 commits August 7, 2023 18:21
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@yebai
Copy link
Member

yebai commented Aug 7, 2023

We could also support Julia 1.6 via Requires. It will also fix failing CIs for Julia 1.6.

@devmotion
Copy link
Member

Alternatively, we could make MCMCChains a regular dependency on Julia < 1.9.

@torfjelde
Copy link
Member Author

Alternatively, we could make MCMCChains a regular dependency on Julia < 1.9.

Is this trivial to do? Not completely obvious to me how the Project.toml looks in this scenario.

@torfjelde
Copy link
Member Author

But I'll make this "merge when ready", and then we can discuss how to deal with < 1.9

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.13% 🎉

Comparison is base (1ebe8bc) 76.22% compared to head (edcac39) 76.35%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #514      +/-   ##
==========================================
+ Coverage   76.22%   76.35%   +0.13%     
==========================================
  Files          23       24       +1     
  Lines        2763     2770       +7     
==========================================
+ Hits         2106     2115       +9     
+ Misses        657      655       -2     
Files Changed Coverage Δ
ext/DynamicPPLMCMCChainsExt.jl 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@devmotion
Copy link
Member

Is this trivial to do? Not completely obvious to me how the Project.toml looks in this scenario.

You just add it also to the [deps] section 🙂

@torfjelde
Copy link
Member Author

You just add it also to the [deps] section slightly_smiling_face

But doesn't this also make it a dep rather than a weakdep on Julia 1.9?

@devmotion
Copy link
Member

No.

@torfjelde
Copy link
Member Author

Ah, I had completely missed this part of the docs; https://pkgdocs.julialang.org/dev/creating-packages/#Transition-from-normal-dependency-to-extension

@torfjelde
Copy link
Member Author

Btw, what's up with the CI now? Didn't we fix this issue of being able to auto-merge at some point? o.O

@torfjelde
Copy link
Member Author

Oh, it seems that removing and re-adding to the merge queue finally triggered the integration tests 👍

@torfjelde torfjelde added this pull request to the merge queue Aug 8, 2023
Merged via the queue into master with commit 4a986df Aug 8, 2023
@torfjelde torfjelde deleted the torfjelde/mcmcchains-ext branch August 8, 2023 08:37
@torfjelde torfjelde mentioned this pull request Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants