Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
subset
andmerge
forVarInfo
(clean version) #544subset
andmerge
forVarInfo
(clean version) #544Changes from 11 commits
028a81a
caa6e25
cac7fa8
5e41c4f
d5a2631
0ade696
db21844
1dbca4c
b67288f
e43029e
cd4033d
8f47dfe
aba9008
3b621ae
2c2c90b
5c1ece3
cfff96c
ed5d948
00c36cf
cf02816
d02cb61
7f01ada
14105e0
c164d32
743162a
dc9ad94
2f320e6
d3a9b56
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
At least a warning?
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.
I will add some more docs for this a bit later today
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.
Remind me the use of
num_produce
?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.
Semantically, taking the sum of two
num_produce
produces no meaning. I think we should throw an error if bothnum_produce
are non-zero.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.
num_produce
is used by the particle samplers to indicate how many observations we've seen / what's the current "observation index".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 likeIf there's no meaning in adding them, then I suggest the alternative is to just give precedence to
varinfo_right
, as is the semantics ofmerge
(if a field/property/key is present in bothleft
andright
, then the value inright
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 thelogp
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).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 what should we do here? Give precedence to
varinfo_right
or leave as it is?Check warning on line 479 in src/varinfo.jl
Codecov / codecov/patch
src/varinfo.jl#L479
Check warning on line 488 in src/varinfo.jl
Codecov / codecov/patch
src/varinfo.jl#L488
Check warning on line 557 in src/varinfo.jl
Codecov / codecov/patch
src/varinfo.jl#L556-L557
Check warning on line 561 in src/varinfo.jl
Codecov / codecov/patch
src/varinfo.jl#L559-L561
Check warning on line 564 in src/varinfo.jl
Codecov / codecov/patch
src/varinfo.jl#L563-L564
Check warning on line 566 in src/varinfo.jl
Codecov / codecov/patch
src/varinfo.jl#L566
Check warning on line 570 in src/varinfo.jl
Codecov / codecov/patch
src/varinfo.jl#L568-L570
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.
I like this API -- we can consider depreciating
num_produce
in favour ofgetorder
in the longer run.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.
I'm confused. Isn't
num_produce
andorder
different things? At least they are two different fields inVarInfo
. I I just added agetorder
method because I've been trying to make the interface ofVarInfo
simpler.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.
num_produce
is used to track the current observation index (for allvn
s), and its current value inserted toVarInfo.metadata
when a newvn
is created.Check warning on line 1678 in src/varinfo.jl
Codecov / codecov/patch
src/varinfo.jl#L1678