Skip to content

Fix WCS distortion keyword handling for alternate WCS keys#19228

Open
Shreyaav18 wants to merge 5 commits intoastropy:mainfrom
Shreyaav18:fix-wcs-distortion-alternate-keys
Open

Fix WCS distortion keyword handling for alternate WCS keys#19228
Shreyaav18 wants to merge 5 commits intoastropy:mainfrom
Shreyaav18:fix-wcs-distortion-alternate-keys

Conversation

@Shreyaav18
Copy link
Copy Markdown

@Shreyaav18 Shreyaav18 commented Jan 27, 2026

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

github-actions bot commented Jan 27, 2026

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 on lines +1197 to +1200
if key == '' or key == ' ':
key_suffix = ''
else:
key_suffix = key
Copy link
Copy Markdown
Contributor

@mcara mcara Jan 27, 2026

Choose a reason for hiding this comment

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

Previous line guarantees that key cannot be ' '.

Suggested change
if key == '' or key == ' ':
key_suffix = ''
else:
key_suffix = key
key_suffix = key

Comment thread astropy/wcs/wcs.py Outdated
)
dp = (d_kw + str(i)).strip()

dp = (d_kw + str(i) + key_suffix).strip()
Copy link
Copy Markdown
Contributor

@mcara mcara Jan 27, 2026

Choose a reason for hiding this comment

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

I don't think strip() is needed here.

Comment thread astropy/wcs/wcs.py Outdated
tables = {}
for i in range(1, self.naxis + 1):
d_error_key = err_kw + str(i)
d_error_key = err_kw + str(i) + key_suffix
Copy link
Copy Markdown
Contributor

@mcara mcara Jan 28, 2026

Choose a reason for hiding this comment

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

Maybe store str(i) + key_suffix as it is reused a couple of times? Just a very minor suggestion.

Comment thread astropy/wcs/wcs.py Outdated
Comment on lines +1263 to +1265
for key_name in set(header):
if key_name.startswith(dp + "."):
del header[key_name]
Copy link
Copy Markdown
Contributor

@mcara mcara Jan 28, 2026

Choose a reason for hiding this comment

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

Please undo this. IMO key is appropriate.

@mcara
Copy link
Copy Markdown
Contributor

mcara commented Jan 28, 2026

Also please fix pre-commit

@Shreyaav18
Copy link
Copy Markdown
Author

Shreyaav18 commented Jan 28, 2026

@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
Copy link
Copy Markdown
Author

Shreyaav18 commented Feb 7, 2026

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

@mcara mcara left a comment •
edited
Loading

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

Shreyaav18 commented Feb 9, 2026

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcara mcara mcara requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

wcs

Projects

None yet

Milestone

v8.0.0

Development

Successfully merging this pull request may close these issues.

Distortion lookup tables on secondary WCSes

3 participants

@Shreyaav18 @mcara @pllim