Support indexing when it removes a dimension of length 1#879
Support indexing when it removes a dimension of length 1#879aaron-kaplan wants to merge 2 commits intozarr-developers:mainfrom
Conversation
aaron-kaplan
temporarily deployed
to
test-release
GitHub Actions
Inactive
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #879 +/- ##
==========================================
- Coverage 89.11% 89.05% -0.06%
==========================================
Files 34 34
Lines 1956 1965 +9
==========================================
+ Hits 1743 1750 +7
- Misses 213 215 +2
🚀 New features to boost your workflow:
|
| from zarr.core.metadata.v3 import ArrayV3Metadata | ||
|
|
||
| from virtualizarr.manifests.array_api import expand_dims | ||
| from .manifest import ChunkManifest |
There was a problem hiding this comment.
Sibling imports avoid loading the parent package's init.py, which sometimes resolves circular import problems. As I write this I can't remember if this was actually necessary in this case. I'll check.
There was a problem hiding this comment.
It shouldn't be. If needed I think you can just import ChunkManifest inside array_selection_1d instead.
|
|
||
| def apply_selection_1d( | ||
| marr: "ManifestArray", indexer_1d: T_BasicIndexer_1d, length: int | ||
| marr: "ManifestArray", indexer_1d: T_BasicIndexer_1d, axis: int, length: int |
There was a problem hiding this comment.
This function signature wasn't dictated by xarray, was it? Seems like it's not implementable without the axis parameter.
There was a problem hiding this comment.
No, these are all implementation details, as xarray only cares about the signatures of certain methods on the ManifestArray class (for indexing __getitem__ is what matters). So feel free to alter the the functions in this file as necessary. (That is one reason I put the indexing internals in this separate indexing.py file.)
| lengths=np.squeeze(marr.manifest._lengths, axis=axis), | ||
| validate_paths=False | ||
| ) | ||
| marr = marr.__class__(metadata, manifest) |
There was a problem hiding this comment.
I would rather say ManifestArray instead of marr.__class__ for clarity, but importing array.py led to a circular import. Seems to me that this function should really be a method of ManifestArray, not split out into a separate file.
There was a problem hiding this comment.
The more conventional fix is to just import ManifestArray inside the function that needs it.
aaron-kaplan
temporarily deployed
to
test-release
GitHub Actions
Inactive
|
Thanks for working on this @aaron-kaplan ! I'm planning a release soon - are you likely to continue working on this or would you rather I picked it up? |
|
@TomNicholas I hoped to get back to it eventually, but I'm not actively working on it right now. If you can do it sooner that would be great. I only addressed the "squeeze" case, but it seems like it other types of indexing should be straightforward to handle as well, as long as they slice along chunk boundaries. |
No problem - I'll probably need to implement this feature for myself soon.
Yes precisely - see #51. |
2 participants
This is to support
xr.DataArray.squeeze. It implements the following comment that was in manifests/indexing.py:I'll raise a couple points as in-line comments.
todo:
docs/releases.rstapi.rst