Skip to content

Fixed bugs with WCS slicing, - #18417

Merged
mcara merged 4 commits into
astropy:mainfrom
astrofrog:fix-wcs-slicing
Jul 14, 2025
Merged

mcara merged 4 commits into
astropy:mainfrom
astrofrog:fix-wcs-slicing

Conversation

@astrofrog

Copy link
Copy Markdown
Member

Specifically slicing WCS objects wi…th more than 2 dimensions but with _naxis not set would fail, and that slicing a WCS with an ellipsis would not return a WCS object but a SlicedFITSWCS instead

Fixes #18410

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@astrofrog
astrofrog requested a review from mcara as a code owner July 10, 2025 12:39
@github-actions github-actions Bot added the wcs label Jul 10, 2025
@github-actions

Copy link
Copy Markdown
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.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@astrofrog astrofrog added this to the v7.1.1 milestone Jul 10, 2025
@pllim pllim added the Extra CI Run cron CI as part of PR label Jul 10, 2025
@mcara

mcara commented Jul 10, 2025

Copy link
Copy Markdown
Contributor

Specifically slicing WCS objects wi…th more than 2 dimensions but with _naxis not set would fail, and that slicing a WCS with an ellipsis would not return a WCS object but a SlicedFITSWCS instead

Pardon my English, but it is not clear to me whether the above is true before this PR or will it be true after this PR will be merged.

@astrofrog

astrofrog commented Jul 10, 2025 •

Copy link
Copy Markdown
Member Author

Sorry for the confusing wording. This PR makes is so we can slice 3+D WCS (it failed before) and makes it so we can do wcs[...] and get back a WCS object (we got back SlicedFITSWCS back before)

@mcara

mcara commented Jul 10, 2025

Copy link
Copy Markdown
Contributor

While it looks like your PR may fix the specific failure, it seems to me it basically is a patch hiding the underlying issue. Based on what I see so far in the code, a lot of things depend on _naxis being set correctly but I feel that code that sets/modifies _naxis is bad. For example, doing this on the 3D wcs object from #18410 results in _naxis = [0, 0] for a 3D object:

>>> wcs.pixel_shape = None
>>> wcs._naxis
[0, 0]

This leads to the following puzzling printout of a 3D WCS (even with your changes):

WCS Keywords

Number of WCS axes: 3
CTYPE : '' '' ''
CRVAL : 0.0 0.0 0.0
CRPIX : 0.0 0.0 0.0
PC1_1 PC1_2 PC1_3  : 1.0 0.0 0.0
PC2_1 PC2_2 PC2_3  : 0.0 1.0 0.0
PC3_1 PC3_2 PC3_3  : 0.0 0.0 1.0
CDELT : 1.0 1.0 1.0
NAXIS : 0  0

Notice NAXIS has only two values even though at the top it reports "Number of WCS axes: 3".

I think a better approach would be to replace _naxis = [0, 0] throughout the code with _naxis = len(self._wcs.crpix) * [0]. I am not sure yet about this approach but it is the first thing I thought about. Of course, _naxis should be updated after WCSBase.__init__() in wcs.py if _get_naxis() was called with _get_naxis(header=None).

@mcara

mcara commented Jul 11, 2025 •

Copy link
Copy Markdown
Contributor

Oh, another bad example:

In [13]: w = WCS(naxis=1)

In [14]: w
Out[14]:
WCS Keywords

Number of WCS axes: 1
CTYPE : ''
CRVAL : 0.0
CRPIX : 0.0
PC1_1  : 1.0
CDELT : 1.0
NAXIS : 0  0

In [15]: w._naxis
Out[15]: [0, 0]

Why is _naxis being forced to be of length 2 in

astropy/astropy/wcs/wcs.py

Lines 3090 to 3091 in ee5a7fe

elif len(_naxis) == 1:
_naxis.append(0)

@ayshih

ayshih commented Jul 11, 2025

Copy link
Copy Markdown
Contributor

See #18405, which fixes/hides some of the above

@mcara mcara mentioned this pull request Jul 11, 2025
1 task
@astrofrog astrofrog reopened this Jul 14, 2025
…th more than 2 dimensions but with _naxis not set would fail, and that slicing a WCS with an ellipsis would not return a WCS object but a SlicedFITSWCS instead

@mcara mcara left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@mcara
mcara merged commit 57f851b into astropy:main Jul 14, 2025
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Jul 14, 2025
pllim pushed a commit to meeseeksmachine/astropy that referenced this pull request Jul 14, 2025
pllim added a commit that referenced this pull request Jul 14, 2025
…417-on-v7.1.x

Backport PR #18417 on branch v7.1.x (Fixed bugs with WCS slicing, )
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Extra CI Run cron CI as part of PR wcs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error with 3D WCS slicing

4 participants