(feat): support ellipsis indexing #1729
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1729 +/- ##
==========================================
- Coverage 86.93% 84.51% -2.43%
==========================================
Files 40 40
Lines 6039 6050 +11
==========================================
- Hits 5250 5113 -137
- Misses 789 937 +148
|
src/anndata/_core/index.py
Outdated
| @@ -130,7 +140,7 @@ def _fix_slice_bounds(s: slice, length: int) -> slice: | |||
| return slice(start, stop, step) | |||
|
|
|||
|
|
|||
| def unpack_index(index: Index) -> tuple[Index1D, Index1D]: | |||
| def unpack_index(index: Index) -> tuple[IndexRest, IndexRest]: | |||
There was a problem hiding this comment.
Shouldn’t this be (Index without Ellipsis) -> tuple[Index1D, Index1D]?
There was a problem hiding this comment.
No, because this can return EllipsisType on either of the entries
There was a problem hiding this comment.
my point is that it probably shouldn’t. see here
There was a problem hiding this comment.
In other words, Index can contain an ellipsis
There was a problem hiding this comment.
Ok, but @flying-sheep then if we want to use unoack_index everywhere, we need to let it accept tuples of length > 2
Co-authored-by: Philipp A. <[email protected]>
There was a problem hiding this comment.
Our index handling isn’t very consistent, and while parts of that make sense (BaseCompressedSparseDataset doesn‘t need to handle string indexing), we should decide what we want. Currently we use unpack_index in 3 places:
- in
anndata._core.index._normalize_indices, you remove the ellipses first, sounpack_indexnever gets an ellipsis passed from there. Rawdoesn’t use_normalize_indices, it seems to have some hacky slimmed down version of it thatraises when an index tuple isn’t len() 2. That might be OK since we want to phase out Raw.BaseCompressedSparseDatasethas an even simpler version that just passes in the index unchanged tounpack_index. This means that trying to index it with something that one doesn’t support breaks with a hard-to-decipher error. That one should probably support Ellipses
so we need to decide:
- should
unpack_indexhandle all cases? or does it only handle preprocessed indices? - how do we reuse code here smartly? We should optimally only remove ellipses in one place of our code base, but still support both classes that allow string indexing and classes that don’t
src/anndata/_core/index.py
Outdated
| @@ -130,7 +140,7 @@ def _fix_slice_bounds(s: slice, length: int) -> slice: | |||
| return slice(start, stop, step) | |||
|
|
|||
|
|
|||
| def unpack_index(index: Index) -> tuple[Index1D, Index1D]: | |||
| def unpack_index(index: Index) -> tuple[IndexRest, IndexRest]: | |||
There was a problem hiding this comment.
my point is that it probably shouldn’t. see here
|
With the exception of |
It should, I added tests for it |
Co-authored-by: Philipp A. <[email protected]>
Co-authored-by: Philipp A. <[email protected]>
…o ig/ellipsis_indexing
* Backport PR #1729: (feat): support ellipsis indexing * (fix): ellipsis type * (fix): patch versions?
|
I changed the milestone since the relnote is |
Not sure if this is a feature or a fix TBH but going with feature and putting it in 0.10 because it addresses a pretty big use-case that broke without it (so in a not-so-strict sense, is a bug)