Attach varname_to_symbol mapping to Chains
#2078
Conversation
|
So I was just looking at this one myself, and I don't think upping the DPPL version should fix this. This PR should have been functional on it's own. |
| @@ -133,10 +134,9 @@ function AbstractMCMC.bundle_samples( | |||
| le = getlogevidence(samples, state, spl) | |||
|
|
|||
| # Set up the info tuple. | |||
| info = (varname_to_symbol = OrderedDict(zip(varnames, varnames_symbol)),) | |||
There was a problem hiding this comment.
Should we make this inclusion optional? Forcing it could be annoying for serialization of the chains, etc. @yebai
There was a problem hiding this comment.
Can we skip this information while serializaing chains? I think we might want to carry such information in chains by default so functions like predict/generated_quantities can work.
There was a problem hiding this comment.
If we skip it in serialization, then those methods won't work after deserialization anyways 😕
And yes I agree it should be enabled by default, but I was also thinking it would be useful to provide a way to disable it.
There was a problem hiding this comment.
Is there any reason why we can't serialise/deserialise VarName?
There was a problem hiding this comment.
You can, but it means that you no longer can do using MCMCChains; deserialize(...) but you need to have done using Turing first. IMO this is quite annoying 😕
There was a problem hiding this comment.
VarName is defined in AbstractPPL, which is lightweight. Can we make MCMCChains depend on AbstractPPL?
There was a problem hiding this comment.
Potentially in the longer run, but I don't think we should do that when MCMCChains doesn't use any functionality from AbstractPPL.
Pull Request Test Coverage Report for Build 6130928900
💛 - Coveralls |
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #2078 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 21 21
Lines 1447 1449 +2
======================================
- Misses 1447 1449 +2
☔ View full report in Codecov by Sentry. |
|
Okay, so I can't actually reproduce the error from the tests 😕 But I've now made it optional to include the |
After TuringLang/DynamicPPL.jl#534 in DynamicPPL.jl, attaching a dict-like mapping from
VarNametoSymbol(as represented in theChains) allows a much improved experience when working withModelandChainstogether, e.g.generated_quantitiesthat can handle things likeCholeskyvariates.