-
Notifications
You must be signed in to change notification settings - Fork 230
Avoid splitting up varnames until absolutely necessary #2632
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
Conversation
|
Turing.jl documentation for PR #2632 is available at: |
c10b636 to
5634122
Compare
Pull Request Test Coverage Report for Build 16550602808Details
💛 - Coveralls |
5634122 to
7c08115
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2632 +/- ##
==========================================
+ Coverage 84.78% 84.88% +0.09%
==========================================
Files 22 22
Lines 1466 1475 +9
==========================================
+ Hits 1243 1252 +9
Misses 223 223 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sunxd3
left a comment
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.
No objection in principle.
Failed tests look unrelated, but may still worth a look?
|
Yupppp, failures are due to Libtask not working on 1.12 yet. Thanks! |
MCMCChains.jl doesn't like vector-valued or matrix-valued variables. Consequently, in Turing we use
DynamicPPL.varname_and_value_leavesto split things likexup intox[1],x[2], ...This is in general a lossy operation since
xcannot be reconstructed from its individual elements.This conversion used to be done when constructing a
Transition. However, there was no need to do it so early. This PR changes it such that it is only done within the_params_to_arraymethod, which is called insidebundle_samples.On its own, this refactor is quite meaningless from the Turing point of view. However, what it does mean is that if we create a new chains type that doesn't need to break variables up, we can define an
AbstractMCMC.bundle_samples(::Vector{<:Transition}, ..., ::NewChainType)which retains the original data structure in theNewChainType, because now theTransitionwill have that richer information. This has implications for the rewrite of MCMCChains that I'm currently working on.