-
Notifications
You must be signed in to change notification settings - Fork 37
Description
DynamicPPL.jl/src/abstract_varinfo.jl
Lines 150 to 154 in e4fa7f2
| """ | |
| getindex(vi::AbstractVarInfo, ::Colon) | |
| Return the current value(s) of `vn` (`vns`) in `vi` in the support of its (their) | |
| distribution(s) as a flattened `Vector`. |
This is clearly not true, we regularly use vi[:] to access a vector of params as stored internally.
julia> using DynamicPPL, Distributions
julia> @model f() = x ~ Beta()
f (generic function with 2 methods)
julia> m = f(); vi = VarInfo(m); vi2 = link(vi, m);
julia> vi[:]
1-element Vector{Float64}:
0.4059923090008866
julia> vi2[:]
1-element Vector{Float64}:
-0.3805580510140428Note that the default implementation
DynamicPPL.jl/src/abstract_varinfo.jl
Line 160 in e4fa7f2
| Base.getindex(vi::AbstractVarInfo, ::Colon) = values_as(vi, Vector) |
just calls getindex_internal:
Line 2117 in e4fa7f2
| values_as(vi::VarInfo, ::Type{Vector}) = copy(getall(vi)) |
Lines 685 to 690 in e4fa7f2
| getall(vi::TypedVarInfo) = reduce(vcat, map(getall, vi.metadata)) | |
| function getall(md::Metadata) | |
| return mapreduce( | |
| Base.Fix1(getindex_internal, md), vcat, md.vns; init=similar(md.vals, 0) | |
| ) | |
| end |
In theory, the simplest fix to this would be to change the docstring to say that it returns possibly-transformed values.
However, it is technically inconsistent that vi[:] gives internal storage (which may be transformed) whereas vi[@varname(x)] will give you the untransformed values. The correct solution should be to fix getindex(vi, :) to give the untransformed values, and any time we need the transformed values, we should be using getindex_internal(vi, :). This will be pretty annoying, because the 'abuse' of vi[:] is everywhere in the codebase.