BUG: allow step=1 in combine_slices for nested SlicedLowLevelWCS - #20488
Open
wahajahmed010 wants to merge 1 commit into
Open
wahajahmed010 wants to merge 1 commit into
wahajahmed010 wants to merge 1 commit into
Conversation
sanitize_slices already accepts an explicit step of 1 on the first slice, but combine_slices raises "Only slices with steps of 1 are supported" for ANY non-None step, including 1. As a result, slicing a SlicedLowLevelWCS a second time with step=1 (or storing step=1 in the first slice and slicing again) raises, even though semantically step=1 is identical to the default step=None. Normalize step=1 to step=None at the top of combine_slices and accept it in the existing step checks. Errors for non-1, non-None steps are preserved. Closes astropy#20487
Contributor
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
astrofrog
requested changes
Sep 28, 2026
astrofrog
left a comment
astrofrog
left a comment
Member
There was a problem hiding this comment.
Minor comment below, but looks good to me otherwise, will approve once this is implemented.
| if isinstance(slice1, slice) and slice1.step == 1: | ||
| slice1 = slice(slice1.start, slice1.stop) | ||
| if isinstance(slice2, slice) and slice2.step == 1: | ||
| slice2 = slice(slice2.start, slice2.stop) |
Member
There was a problem hiding this comment.
I think this block could actually be moved up to just after the docstring and then the other code changes (adding and sliceN.step != 1) might not be needed
This branch has not been deployed
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
3 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.
Fixes #20487
Description
SlicedLowLevelWCSaccepts an explicitstep=1on the first slice becausesanitize_slicesalready treatsstep=1likestep=None(seesanitize_slicesinastropy/wcs/wcsapi/wrappers/sliced_wcs.py). However, the chained-slice code path callscombine_slicesagain, which rejects any non-Nonestep — including the case where the first slice storedstep=1or the new slice usesstep=1.The result is a confusing
ValueError("Only slices with steps of 1 are supported")for code like:This PR makes
combine_slicesacceptstep=1in either argument, matching the contract already enforced bysanitize_slices. Any other step (e.g.2) is still rejected.Changes
astropy/wcs/wcsapi/wrappers/sliced_wcs.py: relax the early step guard incombine_slicesto also allowstep == 1, normalizeslice(..., step=1)toslice(..., step=None)up front, and document the behavior in the docstring.astropy/wcs/wcsapi/wrappers/tests/test_sliced_wcs.py: append 8 new parametrized regression cases to the existingtest_combine_slicestable covering the pairs from the issue reproduction.docs/changes/wcs/20487.bugfix.rst: changelog entry following the repository's fragment convention.AI Disclosure
This PR was prepared with AI assistance. The code change was drafted by an LLM (ollama/minimax-m3, acting as the implementation subagent) and verified offline against the existing parametrized cases in
test_combine_slicesplus 8 new regression cases (28/28 pass, rejection of non-1 steps is preserved). The final patch was reviewed and applied by the human contributor.