Skip to content

Conversation

torfjelde
Copy link
Member

We often find ourselves in situations where we need to flatten both varnames and values together to produce a "simple" map varname => value where value isa Real rather than a Vector or NamedTuple, e.g. in construction of MCMCChains.Chains.

For this we have the function varname_and_value_leaves(vn, x) which takes in a VarName and some value x, e.g. a Vector, and returns an iterator over pairs of VarName and Real as desired:

DynamicPPL.jl/src/utils.jl

Lines 970 to 985 in 0e40bd0

"""
varname_and_value_leaves(vn::VarName, val)
Return an iterator over all varname-value pairs that are represented by `vn` on `val`.
# Examples
```jldoctest varname-and-value-leaves
julia> using DynamicPPL: varname_and_value_leaves
julia> foreach(println, varname_and_value_leaves(@varname(x), 1:2))
(x[1], 1)
(x[2], 2)
julia> foreach(println, varname_and_value_leaves(@varname(x[1:2]), 1:2))
(x[1:2][1], 1)
(x[1:2][2], 2)

But very often we actually want to do this for a container of multiple VarNames, e.g. OrderedDict or NamedTuple. Currently we just do this "by hand" every time we need this, but that seems unnecessary.

Sooo this PR just adds an implementation for OrderedDict and NamedTuple (which are the most common containers we work with, though the impl is very trivial) so we don't have to do this by hand everywhere.

Ref: #663 where we would benefit from this in the tests.

with multiple varnames typically used in the context of DPPL
@coveralls
Copy link

coveralls commented Sep 26, 2024

Pull Request Test Coverage Report for Build 11049301945

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 19 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.03%) to 77.511%

Files with Coverage Reduction New Missed Lines %
src/model.jl 1 88.46%
src/threadsafe.jl 18 49.57%
Totals Coverage Status
Change from base Build 10992907511: 0.03%
Covered Lines: 2716
Relevant Lines: 3504

💛 - Coveralls

@torfjelde torfjelde requested a review from mhauru September 26, 2024 09:50
Copy link
Member

@mhauru mhauru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a question about whether this could be defined more broadly.

@torfjelde torfjelde enabled auto-merge September 30, 2024 08:31
@torfjelde
Copy link
Member Author

Thanks @mhauru !

@torfjelde torfjelde added this pull request to the merge queue Sep 30, 2024
Merged via the queue into master with commit 4c60c9f Sep 30, 2024
11 of 12 checks passed
@torfjelde torfjelde deleted the torfjelde/varname-and-value-leaves-improvement branch September 30, 2024 08:53
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