Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement custom getindex for UntypedVarInfo #704

Merged
merged 2 commits into from
Oct 31, 2024
+2 −0
Merged

Implement custom getindex for UntypedVarInfo #704

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
35e3c34
Implement getindex for UntypedVarInfo
penelopeysm Oct 29, 2024
1384fa5
Format
penelopeysm Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/varinfo.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1694,6 +1694,7 @@

The value(s) may or may not be transformed to Euclidean space.
"""
getindex(vi::UntypedVarInfo, spl::Sampler) = copy(getindex(vi.metadata.vals, _getranges(vi, spl)))

Check warning on line 1697 in src/varinfo.jl

View check run for this annotation

Codecov / codecov/patch

src/varinfo.jl#L1697 

Added line #L1697 was not covered by tests
getindex(vi::VarInfo, spl::Sampler) = copy(getindex_internal(vi, _getranges(vi, spl)))
function getindex(vi::TypedVarInfo, spl::Sampler)
Copy link
Member Author

@penelopeysm penelopeysm Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Gets the ranges as a NamedTuple
Expand Down
Loading