Implement custom getindex for UntypedVarInfo
#704
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #704 +/- ##
==========================================
- Coverage 79.23% 77.73% -1.50%
==========================================
Files 30 30
Lines 4214 3926 -288
==========================================
- Hits 3339 3052 -287
+ Misses 875 874 -1 ☔ View full report in Codecov by Sentry. |
Pull Request Test Coverage Report for Build 11584766197Details
💛 - Coveralls |
penelopeysm
force-pushed
the
py/untyped-varinfo-getindex
branch
from
83d5b65 to
35e3c34
Compare
src/varinfo.jl
Outdated
| getindex(vi::UntypedVarInfo, spl::Sampler) = copy(getindex(vi.metadata.vals, _getranges(vi, spl))) | ||
| getindex(vi::VarInfo, spl::Sampler) = copy(getindex_internal(vi, _getranges(vi, spl))) | ||
| function getindex(vi::TypedVarInfo, spl::Sampler) |
There was a problem hiding this comment.
Not 100% sure whether we still need this method on L1698 (the one with vi::VarInfo), although I think we do as it would still catch VectorVarInfo. I think you all are best placed to comment since you've worked on it :)
FWIW, I removed L1698 and tests still passed locally, so as far as our test suite is concerned it isn't needed. But I don't think that's necessarily the best indication.
I did also consider, instead of implementing getindex, implementing getindex_internal for the appropriate combination of inputs. I didn't entirely like that because the second argument would be Vector{Int64}, which seemed out of place compared to the other implementations of getindex_internal. It seemed 'simpler' to me to directly overload getindex. But totally happy to do otherwise, if there is a semantic difference between the two.
There was a problem hiding this comment.
There is a semantic difference, namely that getindex should get the value transformed back to original space whereas getindex_internal accesses the internal storage directly, but the existing getindex(vi::VarInfo, spl::Sampler) already violates that so I don't think it matters. All the indexing by samplers is on its way out anyway.
I would keep the existing method to catch VectorVarInfo, as you said.
src/varinfo.jl
Outdated
| getindex(vi::UntypedVarInfo, spl::Sampler) = copy(getindex(vi.metadata.vals, _getranges(vi, spl))) | ||
| getindex(vi::VarInfo, spl::Sampler) = copy(getindex_internal(vi, _getranges(vi, spl))) | ||
| function getindex(vi::TypedVarInfo, spl::Sampler) |
There was a problem hiding this comment.
There is a semantic difference, namely that getindex should get the value transformed back to original space whereas getindex_internal accesses the internal storage directly, but the existing getindex(vi::VarInfo, spl::Sampler) already violates that so I don't think it matters. All the indexing by samplers is on its way out anyway.
I would keep the existing method to catch VectorVarInfo, as you said.
Closes #705.
Note that CI on 1.11 will still fail because of #702. However, the
test/turing/varinfo.jltests pass now. Specifically, this test:DynamicPPL.jl/test/turing/varinfo.jl
Lines 264 to 265 in 18af48a