Repository navigation
Fix slicing with negative and out-of-range indices - #979
Merged
Merged
Conversation
This was referenced Sep 27, 2026
alasdairwilson
approved these changes
Oct 16, 2026
alasdairwilson
left a comment
alasdairwilson
left a comment
Member
There was a problem hiding this comment.
LGTM, astropy/astropy#20554 has fixed a lot of this but not all (doesn't raise all these errors) and I still think we make this change to avoid pinning astropy to specific bugfix releases.
4 tasks done
Member
Author
|
MORE UPDATES TO ASTROPY |
4 of 5 tasks
nabobalis
force-pushed
the
negative-indices
branch
from
October 20, 2026 21:04
77f115b to
2110d52
Compare
Cadair
requested changes
Oct 21, 2026
| @@ -0,0 +1 @@ | |||
| Fix slicing `~ndcube.NDCube`, `~ndcube.NDCubeSequence` (including `~ndcube.NDCubeSequence.index_as_cube`), `~ndcube.NDCollection` and `~ndcube.NDMeta` with negative or out-of-range indices, which gave wrong world coordinates or raised errors with axis-aware metadata. A failed slice no longer empties the cube's ``meta``. Slicing to a length-0 axis (e.g. ``cube[2:2]``), or with a step along the common axis of ``index_as_cube``, now raises an `IndexError`. | |||
Member
There was a problem hiding this comment.
I feel like this should be 3 separate changelogs.
| np.testing.assert_array_equal(cropped.data, data[3:13, 1:6]) | ||
|
|
||
|
|
||
| def _time_cube(meta=None): |
Member
There was a problem hiding this comment.
This and the meta that's sometimes passed to it should be fixtures.
| sequence_index, common_axis_index = \ | ||
| utils.sequence.cube_like_index_to_sequence_and_common_axis_indices( | ||
| item[common_axis], common_axis, common_axis_lengths) | ||
| range(sum(common_axis_lengths))[item[common_axis]], common_axis, common_axis_lengths) |
Member
There was a problem hiding this comment.
can you add a comment here explaining that this range()[] trick is resolving negative indexes.
nabobalis
force-pushed
the
negative-indices
branch
from
October 21, 2026 18:30
2110d52 to
c00fe05
Compare
Cadair
reviewed
Oct 22, 2026
Cadair
reviewed
Oct 22, 2026
Cadair
reviewed
Oct 22, 2026
Cadair
reviewed
Oct 22, 2026
NDCubeSlicingMixin.__getitem__ passed negative and out-of-range indices straight to the WCS and NDMeta slicers. Before astropy 7.2.3/8.0.2 the WCS slicer takes negative values as offsets from 0, so cube[-1] and cube[-2:] sliced the WCS at negative pixels; from those versions on it raises for them when the WCS has no array_shape. NDMeta's slicer computed shapes with the wrong sign and raised. Resolve each item against self.shape after sanitize_slices, and size NDMeta axes with len(range(n)[item]). The slice step is dropped: sanitize_slices only lets a step of 1 through, and astropy before 7.2.3/8.0.2 rejects an explicit step when slicing an already-sliced WCS, so cube[1:][:2:1] failed. With NDMeta fixed, NDCollection's own negative-index workaround is no longer needed, and it raised TypeError for slices with a None bound such as collection[-2:], so remove it. NDCubeSequence.index_as_cube now resolves the common-axis index against the total common-axis length; it mishandled e.g. index_as_cube[:, -11:-5] and index_as_cube[:, -1]. Refs sunpy#860 and astropy/astropy#15557.
NDCubeSlicingMixin.__getitem__ took the sliceable meta off the cube before sanitising the item and only put it back after a successful slice, so a failed slice such as cube[::2] or cube[0, 0, 0, 0] left the original cube with meta=None. Strip the meta only after the item has been resolved, and restore it in a finally block.
A WCS cannot describe an empty axis, yet slices such as cube[2:2], cube[3:1], cube[-1:1] and cube[10:] silently returned length-0 cubes. Raise IndexError instead, as proposed in sunpy#860. Skip the check when the cube already has a length-0 axis, so such cubes can still be sliced as before. NDCubeSequence.index_as_cube ignored a step along the common axis, so index_as_cube[:, ::2] returned every row. Raise IndexError like NDCube slicing does.
Cadair
force-pushed
the
negative-indices
branch
from
October 22, 2026 10:01
08c3b56 to
26ff8d1
Compare
Cadair
force-pushed
the
negative-indices
branch
from
October 22, 2026 10:02
26ff8d1 to
1f52334
Compare
Cadair
approved these changes
Oct 22, 2026
Member
|
@meeseeksdev backport to 2.4 |
alasdairwilson
pushed a commit
that referenced
this pull request
Oct 23, 2026
alasdairwilson
added a commit
that referenced
this pull request
Oct 23, 2026
…-on-2.4 Backport PR #979 on branch 2.4 (Fix slicing with negative and out-of-range indices)
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
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.
AI Assistance Disclosure
AI tools were used for: