Repository navigation
Add support for slices in ChunkCachedArray - #2937
Merged
Merged
Conversation
Dimensions not given an indexer in .isel() (e.g. the size-1 mockZ dimension for fields without depth) reach _vindex_get as slice(None), which raised NotImplementedError. Expand each slice into an index array over its own trailing dim, following vectorized indexing semantics, and let _raw_vindex broadcast them against the integer indexers.
erikvansebille
approved these changes
Oct 6, 2026
erikvansebille
left a comment
erikvansebille
left a comment
Member
There was a problem hiding this comment.
Looks good. One comment and one question below
| }, | ||
| ) | ||
| def test_field_without_depth_identical_across_backends(backend: BackendT): | ||
| """Fields without a depth dimension get a size-1 ``mockZ`` dimension, which must |
Member
There was a problem hiding this comment.
should we also check for mockT? Or will that be trivial?
Contributor
Author
There was a problem hiding this comment.
I don't think its worth expanding the test in this way, since we're fixing it fundamentally in the array API level
Comment on lines
-162
to
-167
| if any(isinstance(k, slice) for k in key): | ||
| raise NotImplementedError( | ||
| "ChunkCachedArray does not support slices in vectorized indexers. " | ||
| "Use integer arrays for every dimension instead." | ||
| ) | ||
| return self._raw_vindex(*key) |
Member
There was a problem hiding this comment.
Ah, so now slices are fine again. Nice
Contributor
Author
|
There was one test failure (one hypothesis case that was found a couple PRs ago, only affecting the minimum environment). I still need to find exactly why this failure is happening (it wasn't trivially reproducing) - but it shouldn't delay progress/release here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds support for slicing in ChunkCachedArray
Checklist
mainfor normal development,v3-supportfor v3 support)AI Disclosure
I have tested any AI-generated content in my PR.
I take responsibility for any AI-generated content in my PR.
Describe how you used it (e.g., by pasting your prompt):
Used Claude Code (Claude Opus 5.5). I asked it to read TypeError in ChunkCachedArray: '<' not supported between instances of 'slice' and 'int' #2897 and write a minimal failing example. It traced the failure to the size-1
mockZdimension, which_gather_cornersleaves unindexed, so xarray handsChunkCachedArrayaslice(None). I then asked it to:_vindex_getimplementation, benchmark it, and find/remove code written on the assumption that slices aren't supportedIt also ran the test suite and Hypothesis checks with more examples.