`subset` and `merge` for `VarInfo` (clean version) #544

torfjelde · 2023-10-09T11:50:43Z

I was thinking that #542 would be a quick merge, and so I just based #543 on #542 so I could easily test the new Gibbs in Turing.jl, buuut it seems that wasn't as quick as I thought and so this PR replaces #543 without any dependence on #542.

Hence: see #543 for examples and some comments

torfjelde · 2023-10-09T11:56:44Z

src/varinfo.jl

+"""
+    merge(varinfo_left::VarInfo, varinfo_right::VarInfo)
+
+Merge two `VarInfo` instances into one, giving precedence to `varinfo_right` when reasonable.


I will add some more docs for this a bit later today

codecov · 2023-10-09T12:14:44Z

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (927799f) 81.51% compared to head (d3a9b56) 82.76%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #544      +/-   ##
==========================================
+ Coverage   81.51%   82.76%   +1.24%     
==========================================
  Files          25       25              
  Lines        3003     3180     +177     
==========================================
+ Hits         2448     2632     +184     
+ Misses        555      548       -7

Files	Coverage Δ
src/DynamicPPL.jl	`33.33% <ø> (ø)`
src/test_utils.jl	`86.53% <100.00%> (+0.10%)`	⬆️
src/simple_varinfo.jl	`75.25% <76.47%> (+3.98%)`	⬆️
src/varinfo.jl	`93.31% <96.18%> (+0.53%)`	⬆️
src/abstract_varinfo.jl	`85.84% <36.36%> (-5.34%)`	⬇️
src/threadsafe.jl	`45.61% <60.00%> (+25.20%)`	⬆️

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

torfjelde · 2023-10-10T09:40:57Z

Do any of you want to take a look at this @devmotion @yebai ?

yebai · 2023-10-10T12:16:58Z

I'll take a look later today. @sunxd3 can you also help review this PR?

src/varinfo.jl

sunxd3 · 2023-10-10T12:34:29Z

src/varinfo.jl

+end
+
+function subset(metadata::Metadata, vns::AbstractVector{<:VarName})
+    # TODO: Should we error if `vns` contains a variable that is not in `metadata`?


At least a warning?

sunxd3 · 2023-10-10T12:47:48Z

src/varinfo.jl

+    metadata = merge_metadata(varinfo_left.metadata, varinfo_right.metadata)
+    lp = getlogp(varinfo_left) + getlogp(varinfo_right)
+    # TODO: Is this really the way we want to combine `num_produce`?
+    num_produce = varinfo_left.num_produce[] + varinfo_right.num_produce[]


Remind me the use of num_produce?

Semantically, taking the sum of two num_produce produces no meaning. I think we should throw an error if both num_produce are non-zero.

num_produce is used by the particle samplers to indicate how many observations we've seen / what's the current "observation index".

Semantically, taking the sum of two num_produce produces no meaning. I think we should throw an error if both num_produce are non-zero.

I don't think that's quite right. It makes sense if you go "execute model A and then separately execute model B, and now we want to merge the two"; to ensure that the observation index is correctly, we need to add them, no?

Is this a scenario we want to consider? Probably not. But to me it wasn't obvious what else to do.

No matter, I don't think erroring if num_produce is non-zero is the right way to go. We should allow something like

varinfo_with_num_produce = last(evaluate!!(model, varinfo, context)) merge(varinfo_with_num_produce, varinfo)

If there's no meaning in adding them, then I suggest the alternative is to just give precedence to varinfo_right, as is the semantics of merge (if a field/property/key is present in both left and right, then the value in right takes precedence).

EDIT: It's fair to ask if we should also do that for logp. I went with sum because I was imagining scenarios where we want to do something like execute two (sub-)models in parallel and then merge the resulting varinfos. In such a scenario, we want to add the logp fields together. But we could maybe make this a separate function, e.g. merge_with_add_logp or something (preferably named in a better way).

So what should we do here? Give precedence to varinfo_right or leave as it is?

sunxd3 · 2023-10-10T12:51:17Z

Looks good to me, but I am not very familiar with VarInfo. Left some questions mainly for my understanding.

test/varinfo.jl

yebai

It looks like a lot of additional code to support VarInfo. Is there a plan to drop VarInfo in favour of SimpleVarInfo at some point?

We have nearly everything we need in SimpleVarInfo now. So, the marginal benefit of keeping both VarInfo and SimpleVarInfo is diminishing.

yebai · 2023-10-10T21:35:59Z

src/varinfo.jl

+    metadata = merge_metadata(varinfo_left.metadata, varinfo_right.metadata)
+    lp = getlogp(varinfo_left) + getlogp(varinfo_right)
+    # TODO: Is this really the way we want to combine `num_produce`?
+    num_produce = varinfo_left.num_produce[] + varinfo_right.num_produce[]


Semantically, taking the sum of two num_produce produces no meaning. I think we should throw an error if both num_produce are non-zero.

yebai · 2023-10-10T21:37:17Z

src/varinfo.jl

 end

+"""
+    getorder(vi::VarInfo, vn::VarName)


I like this API -- we can consider depreciating num_produce in favour of getorder in the longer run.

I'm confused. Isn't num_produce and order different things? At least they are two different fields in VarInfo. I I just added a getorder method because I've been trying to make the interface of VarInfo simpler.

num_produce is used to track the current observation index (for all vns), and its current value inserted to VarInfo.metadata when a new vn is created.

torfjelde · 2023-10-12T15:57:40Z

We have nearly everything we need in SimpleVarInfo now. So, the marginal benefit of keeping both VarInfo and SimpleVarInfo is diminishing.

We don't though 😕 VarInfo is much more flexible than SimpleVarInfo is. See #528, and defining such a "vector" (which is going to be very simliar to metadata) is going to be challenging to get right.

Until that has been done, we need to continue supporting VarInfo.

yebai · 2023-10-12T21:08:51Z

See #528, and defining such a "vector" (which is going to be very simliar to metadata) is going to be challenging to get right.

Yes, I liked the VarDict abstraction as a replacement for Metadata. After this VarDict is complete, SimpleVarInfo should be more general and modular than VarInfo.

torfjelde · 2023-10-12T22:44:08Z

Yes, I liked the VarDict abstraction as a replacement for Metadata. After this VarDict is complete, SimpleVarInfo should be more general and modular than VarInfo.

Agreed, but that will take time; it's non-trivial to get right IMO.

devmotion · 2023-10-13T15:02:35Z

src/abstract_varinfo.jl

+# the `eltype` to `VarName`? This might be useful when someone does `[@varname(x[1]), @varname(m)]` which
+# might result in a `Vector{Any}`.
+"""
+    subset(varinfo::AbstractVarInfo, vns::AbstractVector{<:VarName})


If we would not already have so many getindex methods, I would have thought that getindex would be a natural name for this function. But maybe it's still an option?

Then we could have getindex(::AbstractVarInfo, ::AbstractVector{<:VarName}) -> AbstractVarInfo and getindex(::T, ::VarName) -> typeof_varname_variate, similar to [1,2,3][[1,3]] = [1, 3] and [1,2,3][2] = 2.

I'd really like this yes, but I also really don't want to touch getindex in this codebase 😅

Happy to make this a long-term goal or something though!

devmotion · 2023-10-13T15:04:30Z

src/abstract_varinfo.jl

+See docstring of [`subset(varinfo, vns)`](@ref) for examples.
+"""
+function Base.merge(varinfo::AbstractVarInfo, varinfo_others::AbstractVarInfo...)
+    return merge(Base.merge(varinfo, first(varinfo_others)), Base.tail(varinfo_others)...)


It's possible that length(varinfo_others) == 0, and then the function would error it seems?

Ah true; I'll address this.

I meant to add tests for merge with more than two inputs too, but didn't get around to it. Will do that too.

src/abstract_varinfo.jl

src/simple_varinfo.jl

Co-authored-by: David Widmann <[email protected]>

torfjelde · 2023-10-13T16:12:52Z

Okay, so I think this is now properly ready for a potential approval (after comments have been resolved)

…g version

coveralls · 2023-10-13T16:37:14Z

Pull Request Test Coverage Report for Build 6518813946

160 of 184 (86.96%) changed or added relevant lines in 5 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+1.2%) to 82.767%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/simple_varinfo.jl	13	17	76.47%
src/varinfo.jl	126	131	96.18%
src/abstract_varinfo.jl	4	11	36.36%
src/threadsafe.jl	12	20	60.0%

Files with Coverage Reduction	New Missed Lines	%
src/varinfo.jl	1	93.32%

Totals
Change from base Build 6477265825:	1.2%
Covered Lines:	2632
Relevant Lines:	3180

💛 - Coveralls

torfjelde · 2023-10-13T21:56:58Z

Okay, so I added quite a bit of changes to ensure that ThreadSafeVarInfo will also work with all of this. Atm supporting these functionalities for ThreadSafeVarInfo is not really necessary unless we start using merge and subset within models (which I do imagine we'll start doing at some point, as it allows us to easily parallelize even sampling!), but it's always annoying when ThreadSafeVarInfo doesn't actually_ implement the varinfo interface so figured we should add it.

torfjelde · 2023-10-14T16:41:46Z

Test coverage should now be improved 👍

torfjelde · 2023-10-16T12:27:39Z

This is ready for a look-over now btw.

yebai

Thanks, @torfjelde -- it looks good to me!

torfjelde · 2023-10-20T10:17:12Z

@devmotion had some open comments here, so I really would have preferred to have his 👍 on this PR too

torfjelde added 11 commits October 9, 2023 12:46

added subset which can extract a subset of the varinfo

028a81a

added testing of subset for VarInfo

caa6e25

formatting

cac7fa8

added implementation of merge for VarInfo and tests for it

5e41c4f

more tests

d5a2631

formatting

0ade696

improved merge_metadata for NamedTuple inputs

db21844

added proper handling of the vals in subset

1dbca4c

added docs for subset and merge

b67288f

added subset and merge to documentation

e43029e

formatting

cd4033d

torfjelde mentioned this pull request Oct 9, 2023

subset and merge for VarInfo #543

Closed

torfjelde changed the title ~~Added subset and merge for VarInfo~~ subset and merge for VarInfo (clean version) Oct 9, 2023

torfjelde commented Oct 9, 2023

View reviewed changes

yebai requested a review from sunxd3 October 10, 2023 12:16

sunxd3 reviewed Oct 10, 2023

View reviewed changes

test/varinfo.jl Outdated Show resolved Hide resolved

yebai reviewed Oct 10, 2023

View reviewed changes

torfjelde added 5 commits October 13, 2023 15:02

made merge and subset part of the AbstractVarInfo interface

8f47dfe

added implementations subset and merge for SimpleVarInfo

aba9008

follow standard merge semantics where the right one takes precedence

3b621ae

added proper testing of merge and subset for SimpleVarInfo too

2c2c90b

forgotten inclusion in previous commit

5c1ece3

devmotion reviewed Oct 13, 2023

View reviewed changes

src/abstract_varinfo.jl Outdated Show resolved Hide resolved

devmotion reviewed Oct 13, 2023

View reviewed changes

src/simple_varinfo.jl Outdated Show resolved Hide resolved

torfjelde and others added 5 commits October 13, 2023 17:01

Update src/simple_varinfo.jl

cfff96c

Co-authored-by: David Widmann <[email protected]>

remove two-argument impl of merge

ed5d948

formatting

00c36cf

forgot to add more formatting

cf02816

Merge branch 'master' into torfjelde/subset-and-merge

d02cb61

removed 2-arg version of merge for abstract varinfo in favour of 3-ar…

7f01ada

…g version

torfjelde added 4 commits October 13, 2023 22:42

allow inclusion of threadsafe varinfo in setup_varinfos

14105e0

more tests for thread safe varinfo

c164d32

bugfixes for link and invlink methods when using thread safe varinfo

743162a

attempt at fixing docs

dc9ad94

torfjelde added 2 commits October 14, 2023 17:38

fixed missing test coverage

2f320e6

formatting

d3a9b56

yebai approved these changes Oct 16, 2023

View reviewed changes

yebai enabled auto-merge October 19, 2023 13:26

yebai added this pull request to the merge queue Oct 19, 2023

Merged via the queue into master with commit efd9da3 Oct 19, 2023

yebai deleted the torfjelde/subset-and-merge branch October 19, 2023 16:48

torfjelde mentioned this pull request Oct 20, 2023

Patch version bump so we can release #546

Closed

subset and merge for VarInfo (clean version) #544

subset and merge for VarInfo (clean version) #544

Uh oh!

Conversation

torfjelde commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

torfjelde commented Oct 10, 2023

Uh oh!

yebai commented Oct 10, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torfjelde Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunxd3 commented Oct 10, 2023

Uh oh!

Uh oh!

yebai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torfjelde commented Oct 12, 2023

Uh oh!

yebai commented Oct 12, 2023

Uh oh!

torfjelde commented Oct 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

torfjelde commented Oct 13, 2023

Uh oh!

coveralls commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 6518813946

💛 - Coveralls

Uh oh!

torfjelde commented Oct 13, 2023

Uh oh!

torfjelde commented Oct 14, 2023

Uh oh!

torfjelde commented Oct 16, 2023

Uh oh!

yebai left a comment

Choose a reason for hiding this comment

Uh oh!

torfjelde commented Oct 20, 2023

Uh oh!

Reviewers

Assignees

`subset` and `merge` for `VarInfo` (clean version) #544

`subset` and `merge` for `VarInfo` (clean version) #544

torfjelde commented Oct 9, 2023 •

edited

Loading

codecov bot commented Oct 9, 2023 •

edited

Loading

torfjelde Oct 12, 2023 •

edited

Loading

coveralls commented Oct 13, 2023 •

edited

Loading