Wcs compass #15434
Wcs compass #15434
Conversation
emolter
requested review from
astrofrog,
Cadair,
larrybradley and
mcara
as code owners
|
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
|
Please rebase to grab a fix from |
There was a problem hiding this comment.
Thanks!
Does this work on all the WCS variants that adhere to APE 14; for instance, GWCS?
Should add a warning about distortion in the WCS?
And also clarify that your function only works for NAXIS=2 and not, say, a image cube with higher NAXIS but also a spatial component? In that case, if I have a cube but I want to overlay that compass on a slice, how am I expected to make this work?
| The function :meth:`~astropy.wcs.utils.north_pole_angle` calculates the correct angle for this compass, which can easily be displayed using a matplotlib :class:`~matplotlib.mpl_toolkits.axes_grid1.anchored_artistsAnchoredDirectionArrows()` artist. | ||
|
|
||
|
|
||
| .. plot:: |
There was a problem hiding this comment.
Hard for me to tell what this is supposed to show me unless the RTD build is fixed. Would be nice to actually overlay this on imshow since I think that is how this compass is meant to be used?
|
@pllim touched on most issues I wanted to comment on. Most important is that this code will only work on 2D imaging WCS. It should be "generalized" to at least extract imaging axes from a 3D WCS. [minor comment] To keep this in the same API style with other functions in the One thing that makes me pause, is that DS9, for example (at least many many years ago) I believe, when rotating an image North up, computes rotation with regard to the Y-axis. Definitely, STScI software computes position angle (PA) with regard to the Y-axis. So, either the code should be modified to compute orientation relative to Y-axis or add another argument to allow users to select the axis. See some discussion here astropy/pyregion#89 (I forgot most details though). |
|
Some backstory here so that we have it on the record: @emolter submitted this as part of a dotAstro hack day, which is great! It does raise the question how/if we should make sure credit goes to other members of the group though. @emolter, were there other members of the hack day group you think should get credit? We can do some git magic to make sure they get credit for their contributions if you think that's warranted. |
|
I see your points @mcara and @pllim , but this might be a case where we should not let the perfect be the enemy of the good. I.e., we can merge this just for the 2D imaging case, and generalize it later (just so long as there is sufficient clarifications added either in the docs or code that it only works in those cases) |
Ah, I did not know. Kudos and thank you!
As far as git history is concerned, you can credit others via https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors And if I understand the offline conversation correctly (which happened after I submitted my initial review), @bmorris3 is the only one involved who is also a core maintainer. Is he willing to champion this feature and help push this to a state that is mergeable?
It is possible. But...
And now that I think about it more, should a new image test be added? The one that runs through the CircleCI stuff. |
|
I plan to review this shortly! |
There was a problem hiding this comment.
Thanks for this contribution! Just a couple of big picture comments for now:
- I think that we don't need to worry about extending this to non-2D data for now, we have prior examples such as e.g. add_beam which work only with 2D celestial WCSes.
- I think the visualization code itself should be included in astropy and the public API for this should be a single
add_compassfunction that lives alongsideadd_beaminastropy.visualization.wcsaxes - As is the case for all code in WCSAxes, we should generalize this a little to work via the APE-14 WCS API rather than the old FITS WCS class to make sure this also works e.g. with GWCS and so on. I'm happy to help with the last point once the rest of the code is ready.
…o multiple coauthors Co-authored-by: Lindsey Gordon contactlindseycg@gmail.com Co-authored-by: Eilat Glikman eglikman@middlebury.edu
|
Ok, I fixed (hopefully) all the easy stuff here. I also followed the suggestion from @astrofrog to make an I am admittedly a bit lost when it comes to generalizing this to work on other coordinate systems and to the 3-D case. I would be happy to learn, though, if someone is willing to iterate with me. I've also never tried to write an image test, and similarly, I'd be happy to learn but I'd probably need a fair amount of help. |
|
Thanks for updating this PR! I will review this shortly and see how I can help in terms of generalizing. |
There was a problem hiding this comment.
This is looking good so far! I've now done a detailed review to try and get things tidied up and ready to merge ahead of the 6.0 feature freeze next week.
In addition to the comments below, you can add an image test by copying test_scalebar:
Just add a new function below that one called test_compass, and set up a figure with a compass. In fact once you've implemented the corner option mentioned in my comments, you could add four compasses to the figure in different corners, and you could have e.g. one equatorial, one galactic, one with a frame and one changing other visual settings for the compass.
Once that test function exists, you can run:
tox -e py39-test-image-mpl334-cov -- -k test_compass
to run it. The test will fail because we need to add a 'hash' to a certain file but just ignore that for now - open results/fig_comparison.html and you should see the figure. You can then always iterate it to make it look how you want (in fact the easiest is just to first write a simple script to make the plot and fiddle with it then move the code to the test as it'll be more efficient that way).
In terms of generalizing, I think the code here will actually be general enough to work with APE-14 WCSes so don't worry about generalizing more for now. Once the image test is working I'll explain how to test with a more general WCS.
| sep_E=0.04, | ||
| color="white", | ||
| **kwargs, | ||
| ): |
There was a problem hiding this comment.
To match add_beam and add_scalebar, could you add the following options:
corner="top left",
frame=False,
borderpad=0.4,
along with their implementation? Currently there is no easy way to customize the position of the compass.
There was a problem hiding this comment.
I am doing this as suggested, but just a question so I learn best practices. Frameon, pad, and borderpad are all arguments to AnchoredDirectionArrow, so wouldn't this be just as straightforward to do using the **kwargs, without needing them named explicitly as options here? Is the point that you'd have to go down a rabbit-hole of inheritances to see that these are options?
|
@emolter - just to check, is this something you would have time to work on today or tomorrow? (just to know whether we can wrap it up for the 6.0.0 release) |
|
@astrofrog sorry for the lack of responsiveness, unfortunately I couldn't make the time to work on this before the feature freeze. Thanks for your detailed comments; I'm looking through them now, and will try to make some progress on it this afternoon |
| Additional arguments are passed to | ||
| :class:`~matplotlib.mpl_toolkits.axes_grid1.anchored_artists.AnchoredDirectionArrows`. | ||
| """ | ||
| arrow = AnchoredDirectionArrows( |
There was a problem hiding this comment.
The original reason I though it was important that pixel be an input argument is to handle the case where you have either a very large FOV or are very close to the north pole. The corner="upper left" functionality replaces this, but it's not obvious to me how to back out the pixel location: "upper left" just translates to a single integer that can be read by AnchoredDirectionArrows. Going up the inheritance tree, the location is read somewhere within matplotlib.offsetbox.OffsetBox, and it appears that the location of the arrow's origin might change depending on, e.g., the requested size of the labels. This was already a problem in the old implementation because the arrows were being drawn by the artist in the same way, but at least the user could try to get the x,y pixel from which North was computed near to the location of the arrow in the resulting image.
I see a few options:
- Make the arrow, then try to back out the pixel value at the origin of the arrows, then compute the north pole angle and modify/re-make the arrow. To me this appears to be a lot of work and is clunky, unless someone can suggest a cleaner and easier way; however, it would be satisfying in the sense that it would auto-magically get North right even for large images
- Make "pixel" an optional argument, so a user could try to match the location in the image where North is computed with the location where the arrows are getting drawn
- Leave it as-is, i.e. always compute North at (0, 0), add a disclaimer that it won't work for large images and/or near the north pole, and let someone else implement the edge case if they want to
Perhaps you have a different suggestion @astrofrog, just let me know
| @@ -24,6 +28,56 @@ | |||
| } | |||
|
|
|||
|
|
|||
| def add_compass( | |||
There was a problem hiding this comment.
Is it okay to keep this function with the underscore beforehand as an internal use indicator? if so, where should I put the tests for this function? If not, and I should just put the calculation inside add_compass, how should I write a test that ensures the NP angle is computed properly? It doesn't feel like the figure test really does this
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
|
I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks! If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here. |
Description
This pull request is to address using WCS to compute the north direction in pixel space and enable easy plotting of the N-E compass.