Conversation
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.
|
astrofrog
marked this pull request as ready for review
September 23, 2026 14:32
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
While chatting with @Cadair, we realised that while axis_correlation_matrix in APE 14 tells you which world coordinates depend on which pixel coordinates, there is no matrix to give the opposite.
An example that illustrates this is the solar data @Cadair pointed me to, which has:
If we load this into
WCS, thenaxis_correlation_matrixis:This indicates that pixel dimensions 0 and 1 are needed to derive world coordinates 0 (lon), 1 (lat) and 3 (time).
However, for the reverse, in this case we don't actually need time to get pixel coordinates, so the matrix should look different. With this PR, we can now expose and show the matrix indicating which world coordinates each pixel coordinate needs:
(this is the output from SlicedLowLevelWCS which has a nice way to print the matrices)
One subtlety is that this matrix might in some cases be degenerate, and it's not clear if we should expose just one of the matrices, or provide a way to return multiple ones in a way. However I think the cleanest path forward is to say that a given WCS instance should just provide one matrix and be consistent with what
world_to_pixel_valuesdoes, and if there are multiple ways to do the WCS, then whatever is generating the WCSes could generate different WCSes with different options - it is not up to the WCS to have to worry about which one to choose. For FITS-WCS, this is not a problem in any case as the reverse matrix is always unique.This is formally an addition to the APE 14 API, and as we stated in APE 14, the base class here is the authoritative version.
Out of scope: note that in principle, we can then modify e.g.
world_to_pixel_valuesto be able to takeNonefor non-useful coordinates, or provide a way to slice away useless world coordinates, but for now I am leaving these as out of scope for the present PR.AI Disclosure
If AI tools were used to develop this pull request, describe the tools including specific model and version, how they were used, and what content is AI generated. Otherwise enter "N/A".
The original implementation was done by hand but then I used Claude to add tests, find corner cases, fix them, and implement the FITS version by adapting the
axis_correlation_matriximplementation. Everything has been checked and thought about extensively by me.Merge method