Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Convert WCS wrappers to FITS WCS. #649

Merged
merged 13 commits into from
Nov 15, 2023
Merged

Convert WCS wrappers to FITS WCS. #649

merged 13 commits into from
Nov 15, 2023

Conversation

DanRyanIrish
Copy link
Member

PR Description

This PR is a draft of a feature to convert a chain of WCS wrapper classes to a FITS-WCS, provided the base-level WCS is a FITS-WCS and only supported wrapper types are involved. See Issue #648

@DanRyanIrish DanRyanIrish marked this pull request as draft October 28, 2023 12:36
@DanRyanIrish
Copy link
Member Author

@Cadair @jmason86 Thoughts on this approach?

Cadair
Cadair reviewed Oct 28, 2023
View reviewed changes
ndcube/wcs/wcs_tools.py Outdated
return fitswcs


def slice_fitswcs(fitswcs, slice_items):
Copy link
Member

Choose a reason for hiding this comment

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

You might find the slice and sub methods on WCS useful: https://docs.astropy.org/en/stable/api/astropy.wcs.WCS.html#astropy.wcs.WCS.slice

Cadair
Cadair reviewed Oct 28, 2023
View reviewed changes
ndcube/wcs/wcs_tools.py Outdated
return low_level_wrapper
# Determine chain of wrappers down to the FITS-WCS.
wrapper_chain = [type(low_level_wrapper)]
while hasattr(low_level_wrapper, "_wcs"):
Copy link
Member

Choose a reason for hiding this comment

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

Well this is horrifying. I think checking if it's an instance of https://docs.astropy.org/en/stable/api/astropy.wcs.wcsapi.BaseWCSWrapper.html#astropy.wcs.wcsapi.BaseWCSWrapper would be slightly better.

@DanRyanIrish DanRyanIrish force-pushed the unwrap_wcs branch 2 times, most recently from a6a1329 to 6ad941d Compare October 29, 2023 12:48
@DanRyanIrish
Copy link
Member Author

Thanks @Cadair. Here's a 2nd draft. Sticking with the approach for now of not dropping any axes from WCS but letting users know where length-1 dummy axes are needed in the data array for it to be made consistent with the FITS WCS. It saves a lot of logic or error catching about whether WCS.sub can be used. And in cases where one dependent axis is dropped, but not another, this information will be needed anyway.

Thoughts on this draft/approach, @Cadair @jmason86?

@DanRyanIrish DanRyanIrish force-pushed the unwrap_wcs branch 5 times, most recently from 862260a to 4530963 Compare October 30, 2023 13:42
@DanRyanIrish DanRyanIrish added this to the 2.2 milestone Oct 30, 2023
@starfleetjames
Copy link

I took a look (the test in particular was helpful... thanks for writing it so closely to my use case!) and I think this would do the trick!

@DanRyanIrish
Copy link
Member Author

pre-commit.ci autofix

@DanRyanIrish DanRyanIrish marked this pull request as ready for review October 30, 2023 22:25
@DanRyanIrish
Copy link
Member Author

@jmason86 I've released this PR from draft form. So it would be great to get an official review/approval from you.

@Cadair, would you like to review this also now that it's an official proposed PR?

starfleetjames
starfleetjames approved these changes Nov 1, 2023
View reviewed changes
Copy link

@starfleetjames starfleetjames left a comment

Choose a reason for hiding this comment

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

Tested out the code and it indeed appears to operate just as promised!

@DanRyanIrish
Copy link
Member Author

Thanks @jmason86. Can you take a quick look at this line and confirm I have my WCS algebra right? It gives the right answer in the case I've tested. But I have a nagging doubt that it might be a specific solution, rather than a truly general one.

@DanRyanIrish
Copy link
Member Author

@Cadair would you still like to give a review on this? Or can we merge it to get it to @jmason86 ?

Cadair
Cadair approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

Looks good to me, does this need documenting somewhere? Is this even being picked up by our API docs?

ndcube/wcs/wcs_tools.py Outdated Show resolved Hide resolved
ndcube/wcs/wcs_tools.py Outdated Show resolved Hide resolved
@DanRyanIrish
Copy link
Member Author

Is this even being picked up by our API docs?

I don't know. The doc test is failing for some reason. @nabobalis do know why that is? Looks like something is aiohttp is deprecated...

@nabobalis
Copy link
Contributor

It is running on Python/3.12 for some reason. You will want to fix that.

@nabobalis
Copy link
Contributor

pre-commit.ci autofix

@nabobalis
Copy link
Contributor

Might pass now?

@sunpy sunpy deleted a comment from pep8speaks Nov 14, 2023
@nabobalis
Copy link
Contributor

Or not: <unknown>:20: WARNING: py:obj reference target not found: ndcube.wcs.wcs_tools.unwrap_wcs_to_fitswcs

nabobalis
nabobalis reviewed Nov 14, 2023
View reviewed changes
ndcube/wcs/tools.py Outdated Show resolved Hide resolved
nabobalis
nabobalis requested changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

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

Needs a new API page entry. I don't see how it would be pulled in by modapi.

In docs/reference/wcs.rst, I think adding a section for tools should be enough.

Unless the plan is to collapse the namespace?

DanRyanIrish and others added 13 commits November 15, 2023 13:52
@DanRyanIrish @nabobalis
First draft of function to convert WCS wrappers to FITS WCS.
9e5608c
@DanRyanIrish @nabobalis
2nd draft of function to convert WCS wrappers to FITS WCS.
49e93d6
@DanRyanIrish @nabobalis
Add test for unwrap_wcs_to_fitswcs.
730d4a6 
Also fixed some bugs in unwrap_wcs_to_fitswcs found in testing.
@DanRyanIrish @nabobalis
Add changelog for feature 469 and codestyle fixes.
2297324
@DanRyanIrish @nabobalis
Fix bugs in unwrap_wcs_to_fitswcs found in testing.
8445ebd
@pre-commit-ci @nabobalis
[pre-commit.ci] auto fixes from pre-commit.com hooks
2f58ec5 
for more information, see https://pre-commit.ci
@DanRyanIrish @nabobalis
Removes erroneous print statements.
145803f
@DanRyanIrish @nabobalis
Make _slice_fitswcs() handle negative indices when FOTS-WCS doesnt in…
eca6f8e 
…clude axis lengths.
@DanRyanIrish @Cadair @nabobalis
Minor docstring reformatting
927db64 
Co-authored-by: Stuart Mumford <[email protected]>
@DanRyanIrish @nabobalis
rename wcs_tools.py to tools.py
47ffb62
@pre-commit-ci @nabobalis
[pre-commit.ci] auto fixes from pre-commit.com hooks
99bc75a 
for more information, see https://pre-commit.ci
@nabobalis
Apply suggestions from code review
a1820d8
@nabobalis
Fix minor issues
b4edd34
@nabobalis
Copy link
Contributor

Hopefully the CI is passing.

@nabobalis nabobalis merged commit 3639e5c into sunpy:main Nov 15, 2023
12 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nabobalis nabobalis nabobalis approved these changes

@Cadair Cadair Cadair approved these changes

@starfleetjames starfleetjames starfleetjames approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.2
Development

Successfully merging this pull request may close these issues.
4 participants
@DanRyanIrish @starfleetjames @nabobalis @Cadair