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.

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

Breaking: better DimStack indexing, again #689

Merged
merged 2 commits into from
Apr 15, 2024
Merged

Breaking: better DimStack indexing, again #689

merged 2 commits into from
Apr 15, 2024

Conversation

rafaqz
Copy link
Owner

@rafaqz rafaqz commented Apr 8, 2024

This PR adds type parameters K,T,N to stacks for better dispatch and type stabiity in both Array-like and NamedTuple-lke behaviors.

It it incliudes a bunch of performance fixes and more complete tests.

Pacakges extending AbstractDimStack{L} will now need to extend AbstractDimStack{K,T,N,L}. There are new helper methods stacktype and data_eltype to help getting these paremeters. data_eltype can be extended for alternate stack data objects besides NamedTuple.

There is also a new standard behavour: indexing with dimensional objects like a vector of dimension tuples will do a mergedims, because you can index a subset of the dimensions. But indexing with colon or Vector of Int/CartesianIndex will return a raw vector of NamedTuple, not a mergedims DimStack - so there is a way to avoid the overhead of buiding the merged lookups.

@sethaxen this will require some changes to Arviz, maybe you want to review. It would be good to make sure I've covered everything and don't need to make any more breaking changes.

@rafaqz rafaqz requested a review from sethaxen April 8, 2024 21:48
@sethaxen
Copy link
Collaborator

@rafaqz I unfortunately don't have the bandwidth for a full review, but I tested against ArviZ, and with only adding the extra type parameters to InferenceObjects.Dataset, everything works.

The indexing changes aren't a problem. I find all positional indexing of an AbstractDimStack to be nonintuitive, so in our documented examples, we always use keyword-based indexing, which seems to still behave the same with this PR.

@rafaqz
Copy link
Owner Author

rafaqz commented Apr 15, 2024

No worries, my main concern was that it doesn't break your code.

Absolutely, most of the positional indexing now actually calls dimensional indexing underneeath, because its the only thing that makes sense. But some other things like a single colon or range are easier to understand e.g. it makes a vector of namedtuple which is a valid table.

@rafaqz rafaqz merged commit 9d882d2 into main Apr 15, 2024
7 of 8 checks passed
@rafaqz rafaqz deleted the stack_indexing branch April 15, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants