Skip to content

Fix WCSAxes frame transformation for non matching units - #20600

Merged
astrofrog merged 3 commits into
astropy:mainfrom
alasdairwilson:fix-wcsaxes-frame-transform-units
Oct 14, 2026
Merged

astrofrog merged 3 commits into
astropy:mainfrom
alasdairwilson:fix-wcsaxes-frame-transform-units

Conversation

@alasdairwilson

Copy link
Copy Markdown
Contributor

I found this because sunpy has started building a Map's WCS with preserve_units=True so that it keeps the arcsecond units of the FITS header (sunpy/sunpy#8875). Anything drawn from world coordinates, like a quadrangle, onto our wcsaxes then comes out as if it was calculated assuming units in degrees.

This is because WCSAxes.get_transform returns an incorrect transform when given a coordinate frame, if the world units of the axes' WCS are not degrees but the frames otherwise match.

Super simple because CoordinateTransform is already computes same_units so we just need to check it at the same time we check same_frame.

AI Disclosure

N/A

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

@giles

giles Bot commented Oct 14, 2026

Copy link
Copy Markdown

Hi 👋 and thank you for your contribution! 🙏

This pull request is being closed automatically - but don't worry, this is not the end, and we may re-open it.

For some background, we have recently started seeing a rapid increase in the number of pull requests opened. Some of these are from good-faith humans, but some of which are from autonomous LLM agents or humans using LLMs who do not have a genuine understanding of, or interest in, the project. We are therefore auto-closing pull requests from new contributors, but if you are a good-faith human, we want to make sure your pull request gets considered! So if you would like us to re-open your pull request so that it gets reviewed, you need to do two things:

  1. Add a comment here to explain why you need the changes here to be considered, as in how the bug or missing feature affects your work, or whether this is an issue you have encountered but does not affect you.

  2. Join the astropy slack using this invite link and head over to the #new-contributor-requests channel to introduce yourself and let us know about this pull request

The second step is important, as we may otherwise miss notifications about this pull request.

To be clear, our AI policy does allow the use of LLMs as part of contributions, but we need to see authentic engagement and understanding from humans making the contribution.

Notes for maintainers

If you know this is a genuine user and the PR is is compliance with the AI policy, you may re-open this straight away. After two successful merged pull requests, the user will be added to the allowlist and future PRs will no longer be closed automatically.

This user has not made any pull requests to this repository prior to this one.

In addition, here are some statistics on the user's activity on GitHub:

Last 24 hours Last 7 days
Pull requests opened 7 11
Issues opened 0 0

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

@Cadair Cadair reopened this Oct 14, 2026
@Cadair

Cadair commented Oct 14, 2026

Copy link
Copy Markdown
Member

I attest that @alasdairwilson is a human who works on sunpy

astrofrog
astrofrog previously approved these changes Oct 14, 2026

@astrofrog astrofrog left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me, thanks!

Cadair
Cadair previously approved these changes Oct 14, 2026
@astrofrog

Copy link
Copy Markdown
Member

Oh wait no, test_wcsapi_2d_celestial_arcsec is broken

@astrofrog
astrofrog dismissed their stale review October 14, 2026 13:51

Visual tests are broken

@Cadair
Cadair dismissed their stale review October 14, 2026 13:52

tests failing

@astrofrog astrofrog left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks like test_wcsapi_2d_celestial_arcsec is broken in the visual tests, so will need to be fixed before we can merge this.

@astrofrog astrofrog added backport-v7.2.x on-merge: backport to v7.2.x backport-v8.0.x on-merge: backport to v8.0.x labels Oct 14, 2026
@astrofrog astrofrog added this to the v7.2.3 milestone Oct 14, 2026
@astrofrog astrofrog added the Bug label Oct 14, 2026
@alasdairwilson

Copy link
Copy Markdown
Contributor Author

Dang, I'll have a look, I guess it is the inverse problem we were having with deg <-> arcsec

@pllim

pllim commented Oct 14, 2026

Copy link
Copy Markdown
Member

Re: #20600 (comment)

But who can attest for @Cadair ? 😆

@larrybradley

Copy link
Copy Markdown
Member

But who can attest for @Cadair ? 😆

It's bots all the way down... 😆

@bsipocz

bsipocz commented Oct 14, 2026 •

Copy link
Copy Markdown
Member

But who can attest for @Cadair ? 😆

All I can say it's misty hilltop, so the pfp is suspicious.

@alasdairwilson

Copy link
Copy Markdown
Contributor Author

It looks like test_wcsapi_2d_celestial_arcsec is broken in the visual tests, so will need to be fixed before we can merge this.

all good now _transform_plot_args uses get_transform("world") it looks like this was missed when the relaxing to non-degree units was done, it was mistakenly passing degrees to the transform plot coords but that was cancelled out by the fact that the units not mismatching meant the transform wasn't applied?

Or something, can't say I fully understand the process other than I think its fixed now.

@astrofrog astrofrog left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, thank you!

@astrofrog
astrofrog merged commit 5266aa5 into astropy:main Oct 14, 2026
43 of 44 checks passed
astrofrog added a commit that referenced this pull request Oct 14, 2026
…600-on-v8.0.x

Backport PR #20600 on branch v8.0.x (Fix WCSAxes frame transformation for non matching units)
astrofrog added a commit that referenced this pull request Oct 14, 2026
…600-on-v7.2.x

Backport PR #20600 on branch v7.2.x (Fix WCSAxes frame transformation for non matching units)
@alasdairwilson
alasdairwilson deleted the fix-wcsaxes-frame-transform-units branch October 16, 2026 12:36
@astrofrog astrofrog mentioned this pull request Nov 5, 2026
1 of 2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-v7.2.x on-merge: backport to v7.2.x backport-v8.0.x on-merge: backport to v8.0.x Bug visualization.wcsaxes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants