Remove vnt_size and size checks when setting in VNT#1281
Merged
penelopeysm merged 1 commit intobreakingfrom Feb 21, 2026
Merged
Remove vnt_size and size checks when setting in VNT#1281penelopeysm merged 1 commit intobreakingfrom
vnt_size and size checks when setting in VNT#1281penelopeysm merged 1 commit intobreakingfrom
Conversation
Contributor
Benchmark Report
Computer InformationBenchmark Results |
74dbd51 to
97abdd7
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #1281 +/- ##
============================================
+ Coverage 78.76% 78.78% +0.01%
============================================
Files 47 47
Lines 3589 3582 -7
============================================
- Hits 2827 2822 -5
+ Misses 762 760 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
DynamicPPL.jl documentation for PR #1281 is available at: |
bd726c8 to
7325d04
Compare
7325d04 to
2f247e3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR removes size checks when setting a value inside a VNT. That means, for example, you can assign
Dirichlet(ones(2))(which has size(2,)) tox[1:5]even though the indices are not the same size:The reason for this is twofold.
Pointwise log-probabilities
One is to do with #1279. The issue there is that when accumulating pointwise log-densities in VNT, you can have a model that looks like
It's only possible to associate a single float probability with the VarName
x[1:2]. If there were size checks, it would be impossible to associate a true float withx[1:2], because the size of a float is()which doesn't line up with the expected size(2,). You would have to wrap a float in a struct likebefore storing it in a VNT. The problem with this is that when the user expects to get a log-probability, they'll now get this ugly
SizedLogProbthing, which forces them to carry the burden of internal VNT details.Sizes are checked at model runtime anyway
The main reason why we want to avoid storing something of the wrong size is because we want to avoid the possibility of constructing "inconsistent VNTs". Here I use "inconsistent" to mean VNTs precisely like the one above:
In this example, which could arise when e.g. storing the prior distributions of variables, we don't ever want to have a scenario where we collect a prior
Dirichlet(ones(2))that cannot be associated with a valuex[1:5].The thing is, though, that can never happen anyway. If you were to write
x[1:5] ~ Dirichlet(ones(2)), when you evaluate the model, this will fail anyway becauserand(Dirichlet(...))will return a length-2 vector, which cannot then be set intox[1:5]. (This is completely independent of any VNTs and will fail even with an empty OnlyAccsVarInfo that has no accumulators.)In my opinion, then, the size check when setting items in VNTs is superfluous because it is impossible to construct a model that will both run correctly and lead to inconsistent VNTs. Inconsistent VNTs only arise from models that cannot be run.
Conclusion
If not for the issue with pointwise logprobs, I would have probably just let it slide and kept the size check in even though IMO it doesn't accomplish much. However, given that there is actually a motivation to remove it, I decided that it's better to remove it.
Because of this, several structs in DynamicPPL that used to store size information now no longer need to. These include
RangeAndLinked,VectorValue, andLinkedVectorValue. I have therefore also removed those fields.I'm currently somewhere around 95% sure that this is the right thing to do. However, I've decided to separate this into another PR just to cover for that 5% chance, in case I need to revert it in the future.
Previous discussion about the need for size checks
See #1180 (comment).
Given that this was a point of disagreement before, I feel obliged to offer a bit more explanation.
Philosophically I still agree that for a completely generic data structure / container, it makes sense that you cannot assign a value to
x[1:5]unless the value does indeed have size(5,). However, I think in this PR I am rejecting the premise of that argument. Specifically, the use of VNT is so closely tied to a DPPL model (and especially more so since templates / shadow arrays were implemented), that I don't think that VNT is truly a generic data structure, and I think it's okay to rely on point (2) above, and to sacrifice some purity for pragmatism.