Stops using astropy's sanitize_slices by nabobalis · Pull Request #995 · sunpy/ndcube

nabobalis · 2026-10-19T05:15:16Z

PR Description

Closes #994.

astropy 8.0.2 reimplemented sanitize_slices with simplify_basic_index, which rejects a negative index when the shape is unknown, so cube[:, -3:-1] raises (#994); astropy 8.1 deprecates sanitize_slices.

AI Assistance Disclosure

AI tools were used for:

Code generation (e.g., when writing an implementation or fixing a bug)
Test/benchmark generation
Documentation (including examples)
Research and understanding
No AI tools were used

Regardless of AI use, the human contributor remains fully responsible for correctness, design choices, licensing compatibility, and long-term maintainability.

https://claude.ai/code/session_01TutVtk4wvj3M6PHKJLBbKe

alasdairwilson

Why adopt the code rather than calling the replacement with a version check on astropy?

nabobalis · 2026-10-19T15:11:36Z

simplify_basic_index requires a concrete size for every dimension, 8.0.2 is the first release to accept None entries and ndcube's minimum is 6.1. ExtraCoords._getitem_wcs and SkyCoordTableCoordinate.__getitem__ only pass a dimension count and a standalone ExtraCoords has no cube shape.

alasdairwilson · 2026-10-19T15:14:10Z

simplify_basic_index requires a concrete size for every dimension, 8.0.2 is the first release to accept None entries and ndcube's minimum is 6.1. ExtraCoords._getitem_wcs and SkyCoordTableCoordinate.__getitem__ only pass a dimension count and a standalone ExtraCoords has no cube shape.

yeah meant swap between sanitise slices and simplifiy_basic_index for pre and post 8.0.2

nabobalis · 2026-10-19T15:22:59Z

Oh, I didn't put two and two together. I will do that instead.

nabobalis · 2026-10-19T16:49:43Z

I made it worse.

alasdairwilson

Since regular cube slicing rejects non‑unit steps, we should add that same guard to index_as_cube. Using that, slice.indices() returns (start, stop, step), and we only take the start and end (with [:2]) and drop the step before the per‑cube slices reach the cube‑level check. That means index_as_cube[::2] or even ::-1 will silently return every row.

Could you apply the same check step with item[common_axis].step != 1 before before calling slice.indices (and add tests for non unitary steps on index_as_cube)

alasdairwilson · 2026-10-20T18:40:27Z

Since regular cube slicing rejects non‑unit steps, we should add that same guard to index_as_cube. Using that, slice.indices() returns (start, stop, step), and we only take the start and end (with [:2]) and drop the step before the per‑cube slices reach the cube‑level check. That means index_as_cube[::2] or even ::-1 will silently return every row.

Could you apply the same check step with item[common_axis].step != 1 before before calling slice.indices (and add tests for non unitary steps on index_as_cube)

I guess in #979 rather than here?

Whatever you want.

nabobalis · 2026-10-20T18:46:46Z

Well I want to make your life easier. Not mine.

alasdairwilson · 2026-10-20T18:50:06Z

Well I want to make your life easier. Not mine.

do it in 979 then and rebase this ontop

nabobalis · 2026-10-20T21:06:48Z

Should be done now.

pllim · 2026-10-21T14:46:17Z

@astrofrog , should we roll back deprecating sanitize_slices upstream? 🤔

astrofrog · 2026-10-21T14:54:21Z

+
+
+if minversion("astropy", "8.0.2"):
+    def sanitize_slices(slices, ndim):


Maybe keep the compat layer private so you don't have to deprecate it later? 🤣

pllim · 2026-10-21T14:57:49Z

We could also do what we did to clobber in FITS, by changing it to pending deprecation and have it pending for a long time. 😆

astrofrog · 2026-10-21T15:09:32Z

@nabobalis would it be helpful if we hold back from introducing the deprecation until 9.0?

nabobalis · 2026-10-21T15:12:19Z

Nah, I don't see the point. Ndcube has to handle it regardless as it does all this crazy slicing. I wouldn't block any astropy changes based on this.

alasdairwilson

name it _sanitize_slices, and leave it out of all

Vendors in the new simplify_basic_index for old versions of astropy

Cadair · 2026-10-22T11:15:00Z

I have largely re-written this to be a lot more sane.

astrofrog · 2026-10-22T11:18:49Z

    import scipy.interpolate
 except ImportError:
    pass
+from ndcube.utils.misc import simplify_basic_index


I still think this should be private?

astrofrog · 2026-10-22T11:20:03Z

-        item = tuple(slice(*i.indices(n)[:2]) if isinstance(i, slice) else range(n)[i]
-                     for i, n in zip(item, self.shape))
+        # Drop the step: nested WCS slicing rejects an explicit step before astropy 7.2.3/8.0.2.
+        item = tuple(slice(i.start, i.stop) if isinstance(i, slice) else i for i in item)


If you want to avoid having removable code in multiple places, you could patch the vendored version of simplify_basic_index to change steps to None if they are 1, then you don't need this here since any step that is present is definitely not None or 1.

alasdairwilson · 2026-10-22T11:17:20Z


-        # Slice cube.
-        item = tuple(sanitize_slices(item, len(self.shape)))
        # TODO: Remove when astropy>7.2.3


What does this refer to now the surrounding code has changed?

The drop the step part below isn't needed once we have patched-astropy

alasdairwilson · 2026-10-22T11:22:10Z

+        # Error early if a step is passed
+        if any(isinstance(i, slice) and i.step not in (None, 1) for i in item):
+            raise IndexError("Slicing WCS with a step is not supported.")


good, I like this

nabobalis force-pushed the sanitize-slices branch from d1efe66 to 681b8b2 Compare October 19, 2026 05:15

nabobalis changed the title ~~Stop using astropy's deprecated sanitize_slices~~ Stops using astropy's deprecated sanitize_slices Oct 19, 2026

nabobalis force-pushed the sanitize-slices branch from 681b8b2 to b2b76ab Compare October 19, 2026 05:16

alasdairwilson reviewed Oct 19, 2026

View reviewed changes

nabobalis marked this pull request as draft October 19, 2026 16:23

nabobalis force-pushed the sanitize-slices branch from b2b76ab to 4308e94 Compare October 19, 2026 16:34

nabobalis changed the title ~~Stops using astropy's deprecated sanitize_slices~~ Stops using astropy's sanitize_slices Oct 19, 2026

alasdairwilson reviewed Oct 20, 2026

View reviewed changes

nabobalis force-pushed the sanitize-slices branch 3 times, most recently from 7e63481 to 50a5ed2 Compare October 20, 2026 21:04

rosteen mentioned this pull request Oct 21, 2026

TST: The sanitize_slices function is deprecated astropy/specutils#1358

Closed

astrofrog reviewed Oct 21, 2026

View reviewed changes

nabobalis force-pushed the sanitize-slices branch from 50a5ed2 to 2b90d37 Compare October 21, 2026 18:30

Cadair force-pushed the sanitize-slices branch from 2b90d37 to 5f8d58b Compare October 22, 2026 10:09

Cadair added this to the 2.4.3 milestone Oct 22, 2026

alasdairwilson requested changes Oct 22, 2026

View reviewed changes

Cadair force-pushed the sanitize-slices branch from 5f8d58b to 8180030 Compare October 22, 2026 10:48

Cadair force-pushed the sanitize-slices branch from 8180030 to 7f38920 Compare October 22, 2026 10:54

Replace sanitize_slices with simplify_basic_index

5226695

Vendors in the new simplify_basic_index for old versions of astropy

Cadair force-pushed the sanitize-slices branch from 7f38920 to 5226695 Compare October 22, 2026 11:03

Remove more dead code by relying on simplify_basic_index

94a5033

Cadair requested a review from astrofrog October 22, 2026 11:14

Cadair marked this pull request as ready for review October 22, 2026 11:14

Cadair requested a review from alasdairwilson October 22, 2026 11:14

astrofrog suggested changes Oct 22, 2026

View reviewed changes

alasdairwilson approved these changes Oct 22, 2026

View reviewed changes

Make wrapper private

27fbef6

Cadair enabled auto-merge October 22, 2026 12:13

Cadair merged commit 4a1eb94 into sunpy:main Oct 22, 2026
22 of 23 checks passed



		if minversion("astropy", "8.0.2"):
		def sanitize_slices(slices, ndim):

Conversation

nabobalis commented Oct 19, 2026 • edited by Cadair Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

AI Assistance Disclosure

Uh oh!

alasdairwilson left a comment

Choose a reason for hiding this comment

Uh oh!

nabobalis commented Oct 19, 2026

Uh oh!

alasdairwilson commented Oct 19, 2026

Uh oh!

nabobalis commented Oct 19, 2026

Uh oh!

nabobalis commented Oct 19, 2026

Uh oh!

alasdairwilson left a comment

Choose a reason for hiding this comment

Uh oh!

alasdairwilson commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nabobalis commented Oct 20, 2026

Uh oh!

alasdairwilson commented Oct 20, 2026

Uh oh!

nabobalis commented Oct 20, 2026

Uh oh!

pllim commented Oct 21, 2026

Uh oh!

astrofrog Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

pllim commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog commented Oct 21, 2026

Uh oh!

nabobalis commented Oct 21, 2026

Uh oh!

alasdairwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Cadair commented Oct 22, 2026

Uh oh!

astrofrog Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

astrofrog Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

alasdairwilson Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

Cadair Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alasdairwilson Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nabobalis commented Oct 19, 2026 •

edited by Cadair

Loading

alasdairwilson commented Oct 20, 2026 •

edited

Loading

pllim commented Oct 21, 2026 •

edited

Loading