Skip to content

Fix WCS distortion keyword handling for alternate WCS keys - #19228

Merged
pllim merged 6 commits into
astropy:mainfrom
Shreyaav18:fix-wcs-distortion-alternate-keys
Jul 8, 2026
Merged

Fix WCS distortion keyword handling for alternate WCS keys#19228
pllim merged 6 commits into
astropy:mainfrom
Shreyaav18:fix-wcs-distortion-alternate-keys

Conversation

@Shreyaav18

@Shreyaav18 Shreyaav18 commented Jan 27, 2026

Copy link
Copy Markdown
Contributor

Description

This pull request addresses issues with WCS distortion lookup tables on secondary WCSes (alternate WCS with key='A', 'B', etc.).

Problem: When a FITS header contains multiple WCS definitions (primary and alternate), the distortion keywords were not respecting WCS key suffixes as specified in the FITS WCS distortion paper. This caused two issues:

  1. Secondary WCS incorrectly loading primary distortion: A secondary WCS without its own distortion keywords (CPDIS1A, CPDIS2A) would incorrectly load the primary WCS's distortion instead of having none.

  2. Crash when secondary WCS has its own distortion: When secondary distortion keywords were properly defined with suffixes, the code would fail to read them and crash with "Unrecognized/unimplemented distortion function: LOOKUP".

Solution: Modified _read_distortion_kw() to accept a key parameter and apply the appropriate suffix to all distortion-related keywords (CPDIS{i}, CPERR{i}, DP{i}.*), ensuring each WCS only reads its own distortion keywords.

Changes:

  • Added key parameter to _read_distortion_kw() method
  • Applied key suffix to distortion keywords when reading headers
  • Added comprehensive tests covering primary WCS, secondary WCS with/without distortion, and multiple alternate keys

Testing:

  • All existing WCS tests pass
  • New tests verify correct behavior for alternate WCS keys
  • Tested with the example FITS file from the issue report

Fixes #18914

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

@Shreyaav18
Shreyaav18 requested a review from mcara as a code owner January 27, 2026 19:48
@github-actions github-actions Bot added the wcs label Jan 27, 2026
@github-actions

github-actions Bot commented Jan 27, 2026

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.

mcara
mcara approved these changes Jan 28, 2026
View reviewed changes
Comment thread astropy/wcs/wcs.py Outdated
Comment thread astropy/wcs/wcs.py Outdated
Comment thread astropy/wcs/wcs.py Outdated
Comment thread astropy/wcs/wcs.py Outdated
@mcara

This comment was marked as resolved.

Sign in to view
@Shreyaav18

Shreyaav18 commented Jan 28, 2026

Copy link
Copy Markdown
Contributor Author

@mcara Changes addressed. All review comments have been implemented and pre-commit checks are passing locally. Looking forward to your final review!

@pllim pllim added this to the v8.0.0 milestone Jan 28, 2026
@Shreyaav18

Shreyaav18 commented Feb 7, 2026

Copy link
Copy Markdown
Contributor Author

Hi @mcara
Just checking if you've had a chance to review this changes
Happy to make any changes needed. No rush.

mcara
mcara requested changes Feb 9, 2026
View reviewed changes

@mcara mcara left a comment •
edited
Loading

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.

This breaks unit tests in stwcs. We need time to figure out how to proceed.
I am putting this PR temporarily on hold. I still believe this PR is OK.

@Shreyaav18

Shreyaav18 commented Feb 9, 2026

Copy link
Copy Markdown
Contributor Author

I understand the stwcs concern. Happy to help troubleshoot or modify the approach if needed

@astrofrog astrofrog modified the milestones: v8.0.0, v8.1.0 May 5, 2026
@s-goldman s-goldman mentioned this pull request May 28, 2026
Merged
4 tasks
@stscijgbot-hstdp stscijgbot-hstdp mentioned this pull request Jun 1, 2026
Closed
@github-actions github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Jun 27, 2026
@github-actions

This comment was marked as outdated.

Sign in to view
@s-goldman

s-goldman commented Jul 2, 2026

Copy link
Copy Markdown

Our STWCS PR #246 should resolve any issues arising from this change. So we are okay with this proceeding with these changes.

@pllim

This comment was marked as resolved.

Sign in to view
@pllim
pllim force-pushed the fix-wcs-distortion-alternate-keys branch from f4376f7 to 8f7453b Compare July 2, 2026 15:43
@pllim pllim added Bug and removed Close? Tell stale bot that this issue/PR is stale labels Jul 2, 2026
@svank svank mentioned this pull request Jul 2, 2026
Open
mcara
mcara approved these changes Jul 8, 2026
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

@pllim
pllim force-pushed the fix-wcs-distortion-alternate-keys branch from 91f00db to bb3f511 Compare July 8, 2026 14:14
@pllim pllim added the Extra CI Run cron CI as part of PR label Jul 8, 2026
@pllim pllim mentioned this pull request Jul 8, 2026
Closed
@pllim

pllim commented Jul 8, 2026

Copy link
Copy Markdown
Member

linkcheck failure is unrelated. devdeps failures also appear unrelated.

@pllim

pllim commented Jul 8, 2026

Copy link
Copy Markdown
Member

Exotic arch failures aren't related, are they, @astrofrog ?

 In file included from wcsapi_test.c:10:
  .../astropy/wcs/include/astropy_wcs/astropy_wcs_api.h:4:10: fatal error: wcsconfig.h: No such file or directory
      4 | #include "wcsconfig.h"
        |          ^~~~~~~~~~~~~

@pllim

pllim commented Jul 8, 2026

Copy link
Copy Markdown
Member

Exotic arch failures are confirmed to be unrelated. Merging. Thanks!

@pllim
pllim merged commit 82e5cff into astropy:main Jul 8, 2026
47 of 53 checks passed
@svank svank mentioned this pull request Sep 18, 2026
Open
@anishmehta24 anishmehta24 mentioned this pull request Sep 20, 2026
Open
2 tasks
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

Assignees

No one assigned

Labels

Bug Extra CI Run cron CI as part of PR wcs

Projects

None yet

Milestone

v8.1.0

Development

Successfully merging this pull request may close these issues.

Distortion lookup tables on secondary WCSes

5 participants

@Shreyaav18 @mcara @s-goldman @pllim @astrofrog