Use templates in observe pipeline, make pointwise_logprobs return VNT#1279
Use templates in observe pipeline, make pointwise_logprobs return VNT#1279penelopeysm merged 5 commits intobreakingfrom
Conversation
Benchmark Report
Computer InformationBenchmark Results |
| vnt_sz = vnt_size(value) | ||
| if vnt_sz != idx_sz | ||
| throw( | ||
| DimensionMismatch( | ||
| "Assigned value has size $(vnt_sz), which does not match " * | ||
| "the size implied by the indices $(idx_sz).", | ||
| ), | ||
| ) | ||
| end | ||
|
|
||
| # vnt_sz = vnt_size(value) | ||
| # if vnt_sz != idx_sz | ||
| # throw( | ||
| # DimensionMismatch( | ||
| # "Assigned value has size $(vnt_sz), which does not match " * | ||
| # "the size implied by the indices $(idx_sz).", | ||
| # ), | ||
| # ) | ||
| # end |
There was a problem hiding this comment.
This PR also makes a slightly controversial change of removing the check on vnt_size when setting a value in a VNT. The reason is because for pointwise_logdensities, I want to be able to get a VNT that's
x[1:2] => 0.35 # or some other floatbut if you attempt to set 0.35 into the position x[1:2], it will error since 0.35 is a float and has size ().
The usual way around this in DPPL has been to make a wrapper that has the correct size, like
struct FloatWithSize
logp::Float64
size::Tuple
endand then store that. But then you get a VNT that doesn't map x[1:2] to its logprob; you get a VNT that maps x[1:2] to some weird FloatWithSize struct, and then users have to deal with this.
It's fine if we have to deal with wrapper structs internally in DPPL code. But I don't think users should be forced to deal with wrapper structs because of an internal detail of VNT. For the same reason, we don't force users to manually deal with ArrayLikeBlocks either.
So I removed the size check here, essentially allowing any value to be set into any number of indices.
There was a problem hiding this comment.
In principle, this also means the entire vnt_size machinery can be removed, because there's no need for a value to declare its size ahead of time. I haven't done that yet, because I'm still a bit unsure about the removal of size checks. I do think it's the correct thing to do though (I already argued that we should have more flexible size specification way back in #1180 (comment)).
There was a problem hiding this comment.
sounds good to me (sorry for missing the ping)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #1279 +/- ##
============================================
+ Coverage 78.58% 78.76% +0.18%
============================================
Files 47 47
Lines 3600 3589 -11
============================================
- Hits 2829 2827 -2
+ Misses 771 762 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ) where {Prior,Likelihood} | ||
| # vn could be `nothing`, in which case we can't store it in a VNT. | ||
| return if Likelihood && vn isa VarName | ||
| logp = logpdf(right, left) |
There was a problem hiding this comment.
do we still want to keep the support for, e.g.,
julia> logpdf(Normal(), [1.0, 2.0])
2-element Vector{Float64}:
-1.4189385332046727
-2.9189385332046727
julia> loglikelihood(Normal(), [1.0, 2.0])
-4.337877066409345(the old code uses loglikelihood)
There was a problem hiding this comment.
I am a bit confused by the use of this
There was a problem hiding this comment.
Actually, yeah, that is confusing.
There was a problem hiding this comment.
So for something like
@model function f(y)
y ~ Normal()
end
model = f([1.0, 2.0])We need to use loglikelihood inside the LogLikelihoodAccumulator because that expects a single float. But one could easily argue that for pointwise logdensities, it's more informative to return the vector, and thus use logpdf. (Related: #1038)
Using logpdf here would be a departure from the previous behaviour, but if it's more informative I feel like it's a good thing? It is a breaking change, but it is a positive change because it provides strictly more information than before. It would also mean that you could do e.g.
julia> model = f([1.0, 2.0]); v = VarInfo(model);
julia> pointwise_loglikelihoods(model, v)[@varname(y[1])]
-1.4189385332046727There was a problem hiding this comment.
I added a note in the changelog about this.
| vnt_sz = vnt_size(value) | ||
| if vnt_sz != idx_sz | ||
| throw( | ||
| DimensionMismatch( | ||
| "Assigned value has size $(vnt_sz), which does not match " * | ||
| "the size implied by the indices $(idx_sz).", | ||
| ), | ||
| ) | ||
| end | ||
|
|
||
| # vnt_sz = vnt_size(value) | ||
| # if vnt_sz != idx_sz | ||
| # throw( | ||
| # DimensionMismatch( | ||
| # "Assigned value has size $(vnt_sz), which does not match " * | ||
| # "the size implied by the indices $(idx_sz).", | ||
| # ), | ||
| # ) | ||
| # end |
There was a problem hiding this comment.
sounds good to me (sorry for missing the ping)
|
|
||
| ### Pointwise logdensities | ||
|
|
||
| Calling `pointwise_logdensities(model, varinfo)` now returns a `VarNamedTuple` of log-densities rather than an `OrderedDict`. |
There was a problem hiding this comment.
do we want to expose the two optional arguments (i.e. ::Val{Prior} and ::Val{Likelihood})?
There was a problem hiding this comment.
Technically they were always exposed. But actually now thinking about it, I kind of prefer not to expose them, so we would move these arguments to an internal function and make pointwise_logdensities call that with (true, true).
penelopeysm
force-pushed
the
py/templates-in-observe
branch
from
c35e7b1 to
c787174
Compare
|
DynamicPPL.jl documentation for PR #1279 is available at: |
penelopeysm
force-pushed
the
py/templates-in-observe
branch
from
96c6841 to
d2dc47a
Compare
|
I deferred the full removal of size checks in VNT to #1281, but I incorporated the other suggestions. |
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,)`) to `x[1:5]` even though the indices are not the same size: ```julia julia> using DynamicPPL, Distributions julia> vnt = @vnt begin @template x = zeros(5) x[1:5] := Dirichlet(ones(2)) end VarNamedTuple └─ x => PartialArray size=(5,) data::Vector{DynamicPPL.VarNamedTuples.ArrayLikeBlock{Dirichlet{Float64, Vector{Float64}, Float64}, Tuple{UnitRange{Int64}}, @NamedTuple{}, Tuple{Int64}}} ├─ (1,) => DynamicPPL.VarNamedTuples.ArrayLikeBlock{Dirichlet{Float64, Vector{Float64}, Float64}, Tuple{UnitRange{Int64}}, @NamedTuple{}, Tuple{Int64}}(Dirichlet{Float64, Vector{Float64}, Float64}(alpha=[1.0, 1.0]), (1:5,), NamedTuple(), (5,)) ├─ (2,) => DynamicPPL.VarNamedTuples.ArrayLikeBlock{Dirichlet{Float64, Vector{Float64}, Float64}, Tuple{UnitRange{Int64}}, @NamedTuple{}, Tuple{Int64}}(Dirichlet{Float64, Vector{Float64}, Float64}(alpha=[1.0, 1.0]), (1:5,), NamedTuple(), (5,)) ├─ (3,) => DynamicPPL.VarNamedTuples.ArrayLikeBlock{Dirichlet{Float64, Vector{Float64}, Float64}, Tuple{UnitRange{Int64}}, @NamedTuple{}, Tuple{Int64}}(Dirichlet{Float64, Vector{Float64}, Float64}(alpha=[1.0, 1.0]), (1:5,), NamedTuple(), (5,)) ├─ (4,) => DynamicPPL.VarNamedTuples.ArrayLikeBlock{Dirichlet{Float64, Vector{Float64}, Float64}, Tuple{UnitRange{Int64}}, @NamedTuple{}, Tuple{Int64}}(Dirichlet{Float64, Vector{Float64}, Float64}(alpha=[1.0, 1.0]), (1:5,), NamedTuple(), (5,)) └─ (5,) => DynamicPPL.VarNamedTuples.ArrayLikeBlock{Dirichlet{Float64, Vector{Float64}, Float64}, Tuple{UnitRange{Int64}}, @NamedTuple{}, Tuple{Int64}}(Dirichlet{Float64, Vector{Float64}, Float64}(alpha=[1.0, 1.0]), (1:5,), NamedTuple(), (5,)) ``` 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 ```julia @model function ... x[1:2] ~ Dirichlet(ones(2)) end ``` 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 with `x[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 like ```julia struct SizedLogProb{T} lp::Float64 sz::T end vnt_size(s::SizedLogProb) = s.sz ``` before 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 `SizedLogProb` thing, 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: ```julia julia> vnt = @vnt begin @template x = zeros(5) x[1:5] := Dirichlet(ones(2)) end ``` 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 value `x[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 because `rand(Dirichlet(...))` will return a length-2 vector, which cannot then be set into `x[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`, and `LinkedVectorValue`. 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.
2 participants
Closes #1271