-
Notifications
You must be signed in to change notification settings - Fork 29
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
Replacing tonamedtuple
#526
Conversation
0153a99
to
0f4914d
Compare
0f4914d
to
5a43012
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #526 +/- ##
==========================================
+ Coverage 80.40% 80.56% +0.16%
==========================================
Files 24 24
Lines 2776 2799 +23
==========================================
+ Hits 2232 2255 +23
Misses 544 544
☔ View full report in Codecov by Sentry. |
Pull Request Test Coverage Report for Build 6048260621
💛 - Coveralls |
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.
Good work - please take a look below for a few clarity comments.
""" | ||
varname_and_value_leaves(vn::VarName, val) | ||
|
||
Return an iterator over all varname-value pairs that are represented by `vn` on `val`. |
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.
Return an iterator over all varname-value pairs that are represented by `vn` on `val`. | |
Return an iterator over all (varname::VarName, value::Real) pairs represented by `vn` on `val`. Common types for `val`, including Array, NamedTuple, Real and Cholesky, are supported by default. Handling for new types can be added by overloading `varname_and_value_leaves` methods. |
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.
So I specifically didn't add any documentation about this, as we'd then also have to add docs to varname_and_value_leaves_inner
.
IMO this should be a user-facing method and thus should be "simple" in what it describes (since we are exporting it). People who would potentially be interested in overloading this can eaisly just look at the source code and immediately understand that they need to overload varnames_and_value_leaves_inner
for their type.
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.
And it doesn't always return (varname:::VarName, value::Real)
btw; value
could also be Missing
.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Disabled and re-enabled auto merging after new commits. |
This PR introduces
varname_and_value_leaves
which is similar tovarname_leaves
: it returns a flattened iterator over pairs(varname, value)
in such a way thatvalue
is aReal
.See the docs for examples.
Equipped with this, we can effectively remove all usage of
tonamedtuple
in Turing.jl which has the benefits of:MCMCChains.Chains
or similar (also opening the potential of maybe usingVarName
in place of strings to preserve more structure in the keys).Together with #525 we this will allow significant simplifications in Turing.jl when it comes to moving from a
Turing.Inference.Transition
toMCMCChains.Chains
(or similars) by using immutableinvlink
(introduced in #525) andvarname_and_value_leaves
(introduced in this PR).