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.
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
Add support for custom coordinate frames in WCSAxes specifically in transform_coord_meta_from_wcs #16347
base: main
Are you sure you want to change the base?
Conversation
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to Astropy 👋 and congratulations on your first pull request! 🎉
A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.
If you feel that this pull request has not been responded to in a timely manner, please send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail feedback@astropy.org.
👋 Thank you for your draft pull request! Do you know that you can use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution ! I'll leave it to wcsaxes maintainers to discuss whether this approach is acceptable or not, but here's a quick technical review.
Even though the code might be covered already, I believe a test would also be needed to demonstrate the new feature (and to make sure we don't break it in the future).
You'll also need to provide a change log entry (see docs/changes/README.md
). Thanks !
Co-authored-by: Clément Robert <cr52@protonmail.com>
a3b1be7
to
c28b372
Compare
c28b372
to
e326827
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good! Just one comment below.
|
||
astropy.visualization.wcsaxes.wcsapi.CUSTOM_UCD_COORD_META_MAPPING.update( | ||
myframe_mapping | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should avoid having this be the API to register custom mappings, and instead write a context manager similar to custom_ctype_to_ucd_mapping
to make sure the changes are temporary and cleaned up after use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also have the benefit of allowing this functionality to be documented via the API docs even if we don't add something to the main narrative docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great thanks for the feedback, I've taken a first crack at adding a context manager similar custom_ctype_to_ucd_mapping
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I don't think we should keep the test of modifying the dictionary, we should just be testing the context manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Just a few small comments.
|
||
astropy.visualization.wcsaxes.wcsapi.CUSTOM_UCD_COORD_META_MAPPING.update( | ||
myframe_mapping | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I don't think we should keep the test of modifying the dictionary, we should just be testing the context manager.
self.mapping = {} | ||
for k, v in mapping.items(): | ||
if k in CUSTOM_UCD_COORD_META_MAPPING: | ||
raise ValueError(f"UCD metadata mapping {k} already exists.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we might want to allow existing entries to be overriden, but if this is too much work we could always keep as-is for now and consider it a future improvement to allow overriding.
ax.coords | ||
assert ax.coords["eggs"].coord_type == "longitude" | ||
assert ax.coords["eggs"].coord_wrap == 360 * u.deg | ||
assert ax.coords["eggs"].get_format_unit() == u.arcsec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good if possible to add a test of two nested custom_ucd_wcscoord_mapping
calls to make sure both apply at the same time.
Description
This pull request adds support for externally created customs coordinate frames to WCSAxes by altering how
transform_coord_meta_from_wcs
operates.CUSTOM_UCD_COORD_META_MAPPING
which could be extended by external packages.If this approach is ok then the custom sunpy frame could be moved into the sunpy repo.
I didn't add a new test as while there is new code the current tests cover it I think.In an external package I could register new CTYPE to UCD and UCD to coord meta data mappings.
Fixes #16339