Skip to content

Write distortion keywords with the WCS key suffix in to_fits() - #20446

Open
anishmehta24 wants to merge 4 commits into
astropy:mainfrom
anishmehta24:wcs-secondary-distortion-to-fits
Open

Write distortion keywords with the WCS key suffix in to_fits()#20446
anishmehta24 wants to merge 4 commits into
astropy:mainfrom
anishmehta24:wcs-secondary-distortion-to-fits

Conversation

@anishmehta24

@anishmehta24 anishmehta24 commented Sep 20, 2026 •
edited
Loading

Copy link
Copy Markdown

Description

This pull request is to address WCS.to_fits() writing the lookup-table distortion keywords without the alternate-WCS key suffix. Since #19228 _read_distortion_kw() looks for CPDIS1A / DP1A.* when a WCS is loaded with key="A", but _write_distortion_kw() always wrote CPDIS1 / DP1.*, so the distortion of a secondary WCS did not survive to_fits() followed by WCS(hdulist[0].header, hdulist, key="A") (the round trip in the issue).

Changes:

  • _write_distortion_kw(hdulist, dist="CPDIS", key=" ") appends the stripped, upper-cased key to the axis number the same way _read_distortion_kw() builds its keywords (CPDIS1A, DP1A.EXTVER, DP1A.NAXES, DP1A.AXIS.n, and the matching WCSDVARR EXTVER).
  • to_fits() passes the explicit key when given, and otherwise the object's own key (self.wcs.alt), which is also what to_header() uses for the rest of the header. The primary WCS (key=" ") output is byte-for-byte unchanged.

Test: test_alternate_wcs_distortion_to_fits_roundtrip, parametrized over " " and "A", builds a WCS with two lookup tables, writes it with to_fits(), checks the suffixed keywords (and that no unsuffixed ones are written for "A"), reads it back with the same key and compares the tables, and checks that an explicit to_fits(key="B") wins over the object's key. The "A" case fails on main with KeyError: "Keyword 'CPDIS1A' not found.".

astropy/wcs/tests pass locally (468 passed, 21 skipped) with a source build against wcslib bundled in the repo.

Fixes #20441

AI Disclosure

Claude Code (Claude Opus 5, claude-opus-5) was used, driven by me, to trace the read/write asymmetry from the issue, write the change in _write_distortion_kw / to_fits, the regression test, and the first draft of this description; I reviewed and ran everything. The code and the test as submitted are AI generated.

  • I certify that I am human and that I take full responsibility for this pull request including all interactions with reviewers.

Merge method

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

🤖 Generated with Claude Code

@anishmehta24
Write distortion keywords with the WCS key suffix in to_fits()
d0b7f8d 
WCS.to_fits() wrote CPDISn / DPn.* for every WCS key, while
_read_distortion_kw() looks up CPDISna / DPna.* for an alternate key
since astropy#19228, so a secondary WCS's lookup-table distortion did not
survive a round trip. _write_distortion_kw() now takes the key and
to_fits() passes the explicit key or the object's own key.
@anishmehta24
anishmehta24 requested a review from mcara as a code owner September 20, 2026 23:12
@github-actions github-actions Bot added the wcs label Sep 20, 2026
@github-actions

github-actions Bot commented Sep 20, 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.

@anishmehta24 @claude
Apply ruff format to test_wcs.py
6887901 
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
@pllim

This comment was marked as resolved.

Sign in to view
@pllim pllim added this to the v8.1.0 milestone Sep 21, 2026
@anishmehta24

anishmehta24 commented Sep 21, 2026

Copy link
Copy Markdown
Author

Yes, I am. Sorry, I missed the box; it is checked now.

@mcara

mcara commented Sep 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

This looks good to me. I just need to run regression tests before formal approval.

Drizzlepac: https://github.com/spacetelescope/RegressionTests/actions/runs/35685694447
Reference Drizzlepac (astropy@main): https://github.com/spacetelescope/RegressionTests/actions/runs/35962151675
JWST: https://github.com/spacetelescope/RegressionTests/actions/runs/35685779741
Reference JWST (astropy@main): https://github.com/spacetelescope/RegressionTests/actions/runs/35962445903

One stwcs unit test already fails and I need to investigate whether it is a bug (brought to light by this PR) that we can fix or we need to update stwcs for this upcoming breaking change.

pllim
pllim reviewed Sep 22, 2026
View reviewed changes
Comment thread docs/changes/wcs/20446.bugfix.rst Outdated
@pllim pllim added the Bug label Sep 22, 2026
@pllim pllim mentioned this pull request Sep 22, 2026
Open
@anishmehta24 @claude
Mark up identifiers in the changelog fragment
325e863 
Applies @pllim's suggested change.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01WyCUJcDPnCnQMmuDwsnDmD
@pllim

pllim commented Sep 22, 2026

Copy link
Copy Markdown
Member

Instead of wasting compute resources with LLM, you could have just clicked a button and accept my suggested changes...

@d-lamb d-lamb mentioned this pull request Sep 25, 2026
Open
3 tasks

This branch has not been deployed

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

Reviewers

@pllim pllim pllim left review comments
@mcara mcara Awaiting requested review from mcara mcara is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Bug wcs

Projects

None yet

Milestone

v8.1.0

Development

See More

Successfully merging this pull request may close these issues.

Writing secondary WCSes with distortion keys

3 participants

@anishmehta24 @pllim @mcara