Immutable versions of link and invlink
#525
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #525 +/- ##
==========================================
+ Coverage 80.40% 80.81% +0.41%
==========================================
Files 24 24
Lines 2776 2904 +128
==========================================
+ Hits 2232 2347 +115
- Misses 544 557 +13
☔ View full report in Codecov by Sentry. |
Pull Request Test Coverage Report for Build 6041688975
💛 - Coveralls |
|
|
||
| See also: [`default_transformation`](@ref), [`link`](@ref). | ||
| """ | ||
| invlink(vi::AbstractVarInfo, model::Model) = invlink(vi, SampleFromPrior(), model) |
There was a problem hiding this comment.
SampleFromPrior was introduced before we had the mechanism for passing the t::Transformation. It is now no longer necessary. In the longer run, we can consider replacing SampleFromPrior with a suitable context for clarity and consistency.
| invlink(vi::AbstractVarInfo, model::Model) = invlink(vi, SampleFromPrior(), model) | |
| invlink(vi::AbstractVarInfo, model::Model) = invlink(vi, SampleFromPrior(), model) |
There was a problem hiding this comment.
Yep! This has always been the idea:) But for now, we're not quite read to do that.
src/utils.jl
Outdated
| @@ -514,6 +514,13 @@ function BangBang.possible( | |||
| return BangBang.implements(setindex!, C) && | |||
| promote_type(eltype(C), eltype(T)) <: eltype(C) | |||
| end | |||
| # NOTE: Makes it possible to use ranges, etc. for setting a vector. | |||
There was a problem hiding this comment.
This code is incomprehensible to me. Maybe @sunxd3 can check its correctness.
There was a problem hiding this comment.
Added a more descriptive comment here; hopefully that clarifies things a bit.
There was a problem hiding this comment.
Unfortunately, I think the issue is still there
julia> using DynamicPPL, BangBang
julia> svi = SimpleVarInfo(Dict((@varname(a))=>[1 2; 3 4]))
SimpleVarInfo(Dict(a => [1 2; 3 4]), 0.0)
julia> setindex!!(svi, [2 2; 2 2], @varname(a[1:2, 1:2]))
SimpleVarInfo(Dict{VarName{:a, Setfield.IdentityLens}, Matrix{Any}}(a => [2 2; 2 2]), 0.0)@torfjelde if the memory is still fresh, maybe this is an easy fix? I can help also.
There was a problem hiding this comment.
@torfjelde, for clarity, was this issue fixed?
There was a problem hiding this comment.
Unfortunately, I think the issue is still there
This is a separate issue to the one I fixed though; I specifically only fixed the one for Vector and ranges. We need a more general fix to address all types of arrays. Could you open an issue @sunxd3?
There was a problem hiding this comment.
Also worth pointing out that this has nothing to do with the intrduced functionality in this PR; these bugs are from before.
There was a problem hiding this comment.
Yep, that's why I didn't insist. I'll open an issue.
Co-authored-by: Hong Ge <[email protected]>
`possible` overloads
…methods' into torfjelde/immutable-versions-of-methods
|
Question about |
Basically just used in places like this DynamicPPL.jl/src/context_implementations.jl Lines 202 to 236 in e2178c6 But now we have |
For a while now I've been thinking that it would be nice with immutable and mutable versions of the different
!!functions we have (the!!convention is supposed to mean "prefer mutation").When we introduced the usage of
!!, we simply said that the types themselves would indicate whether or not we were mutating, but often it's convenient for a user to be able to specify that they want immutability, even if the underlying type supports mutability (which would then be used in the corresponding!!method).One particular aspect where immutable versions are convenient is that of
linkandinvlink. For example, before returning the a transition to the user (and potentially aChains), in Turing.jl we always want to make sure that the underlying varinfo has been bothinvlink-ed anddeepcopy-ed so that a) the user sees the samples in constrained space rather than unconstrained, and b) to avoid accidentally passing back buffers which are then mutated in the evaluation of the samples (we've had several bugs where we forget a deepcopy + use mutableinvlink!, resulting in chains being all the same value). With an immutableinvlink, makes such mistakes becomes more difficult.Moreover, currently it's not possible with the mutable invlinking:
This is because
modelcontains distributions that are projected onto a smaller space upon linking, e.g.DirichletorLKJCholesky. Hereunflattenwill only extract the sub-part ofvarinfo.metadata.$var.valsthat is "active" afterlink!!, and then our subsequent call toinvlink!!tries to push a larger-dimensional vector into the too small container.Note that this pattern is quite common in Turing.jl, e.g. https://github.com/TuringLang/Turing.jl/blob/0ad7368384e0885c5a76f5a43d1e6b630ef09926/src/inference/hmc.jl#L212-L222.
Fixing this in the mutable
invlink!!forVarInfois quite awkward + worsens performance, as we will have to resize the underlyingvarinfo.metadata.$var.valsas needed, and then insert the values in the buffer. This is also partially because we've had to shoehorn support for these things intoVarInfo.In an immutable
invlink, implementing this is trivial (as is done in this PR).Related: #504