Skip to content

Add a test for the reporting of the shape of a 1D WCS - #18405

Merged
pllim merged 1 commit into
astropy:mainfrom
ayshih:wcs_1d
Jul 14, 2025
Merged

Add a test for the reporting of the shape of a 1D WCS#18405
pllim merged 1 commit into
astropy:mainfrom
ayshih:wcs_1d

Conversation

@ayshih

@ayshih ayshih commented Jul 8, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

After the changes in #18419, this PR has been reduced to simply adding a test.

Original PR text:

Description

This PR fixes the reporting of shape of a 1D WCS. The shape of a 1D WCS when read from a header is set to a 2D shape with the second pixel dimension (NAXIS2) being zero. This appears to be vestigial: this approach was originally to support a _naxis2 property (#5411 from 2016), but that property has long since been removed (#9465 from 2019).

Before this PR:

>>> from astropy.wcs import WCS
>>> wcs = WCS({
...     'naxis1': 10,
...     'crval1': 1,
...     'cdelt1': 0.1,
...     'crpix1': 1,
... })
>>> wcs
WCS Keywords

Number of WCS axes: 1
CTYPE : ''
CRVAL : 1.0
CRPIX : 1.0
PC1_1  : 1.0
CDELT : 0.1
NAXIS : 10  0
>>> wcs.pixel_shape
(10, 0)

After this PR:

>>> from astropy.wcs import WCS
>>> wcs = WCS({
...     'naxis1': 10,
...     'crval1': 1,
...     'cdelt1': 0.1,
...     'crpix1': 1,
... })
>>> wcs
WCS Keywords

Number of WCS axes: 1
CTYPE : ''
CRVAL : 1.0
CRPIX : 1.0
PC1_1  : 1.0
CDELT : 0.1
NAXIS : 10
>>> wcs.pixel_shape
(10,)

This PR also makes a change to the repr of the WCS instance. When the shape is undefined in the header or is cleared, the private attribute (WCS._naxis) gets set to the sentinel value of [0, 0] (irrespective of the number of pixel dimensions). Public API like WCS.pixel_shape check for the sentinel value, but the repr directly accesses self._naxis, so the puzzling line NAXIS : 0 0 appears in the repr. I think changing the sentinel-value approach could break code in the wild, so this PR takes the simple approach of checking for the sentinel value before printing out the NAXIS: ... line.

  • 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.

@github-actions github-actions Bot added the wcs label Jul 8, 2025
@github-actions

github-actions Bot commented Jul 8, 2025

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.

@github-actions

github-actions Bot commented Jul 8, 2025

Copy link
Copy Markdown
Contributor

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@pllim pllim added this to the v7.1.1 milestone Jul 8, 2025
@ayshih
ayshih marked this pull request as ready for review July 8, 2025 17:48
@ayshih
ayshih requested a review from mcara as a code owner July 8, 2025 17:48
This was referenced Jul 9, 2025
Closed
Merged
@mcara mcara mentioned this pull request Jul 11, 2025
Merged
1 task
@astrofrog astrofrog reopened this Jul 14, 2025
@astrofrog

astrofrog commented Jul 14, 2025

Copy link
Copy Markdown
Member

@ayshih - is this still needed? If so, could you rebase? (at least would be good to keep the test?)

@mcara

mcara commented Jul 14, 2025

Copy link
Copy Markdown
Contributor

Personally, I think it is not needed as it helps my brain to see NAXIS: 0 0. If NAXIS will not be printed I wonder if it will make people wonder why it was not printed instead of connecting it to the fact that NAXIS was not set. If anything, I would favor NAXIS: NOT SET or NAXIS: UNDEFINED instead of removing this line altogether.

@pllim pllim added the Close? Tell stale bot that this issue/PR is stale label Jul 14, 2025
@ayshih

ayshih commented Jul 14, 2025

Copy link
Copy Markdown
Contributor Author

(at least would be good to keep the test?)

Agreed, I've adjusted this PR so it's now only adding a test.

Personally, I think it is not needed as it helps my brain to see NAXIS: 0 0.

I think the NAXIS line is fine now that #18419 fixed the problematic aspect. That is, for a 1D WCS, the repr now reports NAXIS: 0 instead of the confusing NAXIS: 0 0.

@ayshih ayshih changed the title Fixed the reporting of the shape of a 1D WCS Add a test for the reporting of the shape of a 1D WCS Jul 14, 2025
@pllim pllim added testing no-changelog-entry-needed and removed Close? Tell stale bot that this issue/PR is stale labels Jul 14, 2025
@pllim
pllim requested a review from astrofrog July 14, 2025 15:18
mcara
mcara approved these changes Jul 14, 2025
View reviewed changes

@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!

@pllim
pllim merged commit 769b06a into astropy:main Jul 14, 2025
@pllim

pllim commented Jul 14, 2025

Copy link
Copy Markdown
Member

Thanks, all!

meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Jul 14, 2025
@pllim @meeseeksmachine
Backport PR astropy#18405: Add a test for the reporting of the shape …
d85f1b2 
…of a 1D WCS
@meeseeksmachine meeseeksmachine mentioned this pull request Jul 14, 2025
Merged
pllim added a commit that referenced this pull request Jul 14, 2025
@pllim
Merge pull request #18432 from meeseeksmachine/auto-backport-of-pr-18…
2f87e10 
…405-on-v7.1.x

Backport PR #18405 on branch v7.1.x (Add a test for the reporting of the shape of a 1D WCS)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcara mcara mcara approved these changes
@astrofrog astrofrog Awaiting requested review from astrofrog

Assignees

No one assigned

Labels

no-changelog-entry-needed testing wcs

Projects

None yet

Milestone

v7.1.1

Development

Successfully merging this pull request may close these issues.

4 participants

@ayshih @astrofrog @mcara @pllim