-
-
Notifications
You must be signed in to change notification settings - Fork 17
Conversation
API Document for WCSAxes (v2)
ax[other_wcs].contour(scuba_image) | ||
|
||
Note that ``ax[other_wcs].imshow()`` would not work (due to matplotlib | ||
limitations). |
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.
What about ax[other_wcs].pcolormesh
and its ilk?
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 that would work because it works by plotting polygons. It's just that imshow
doesn't support transformations.
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.
In the case pywcsgrid2, ax[other_wcs] returns a new axes but with different transform. So, everything vector-based is supposed to work. Showing an image is quite different though.
pcolormesh, which is vector based, should work. But pcolormesh is quite slow unless you use agg backends.
Matplotlib now supports affine-transformed images (not for every backend but most of them), so we can support imshow by locally approximating the transform as an affine-transform. And my experience is that this is enough for most cases, but will not applicable for things like allsky plot, for example. Or we can regrid the image on the fly of course.
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.
Arbitrary image resampling (a la drizzle
) has been on the TODO list for matplotlib for a long time. I'd love to find the time to do it (or find someone else's time to do it). In the meantime, I think, as @leejjoon points out, our best bets are to approximate with affines or use pcolormesh if the transform is too non-linear.
Oh -- and bonus points for providing a method that will use one or the other depending on whether the error rate of using imshow is above some threshold.
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.
On this front, cartopy can handle inter-coordinate reference system transformations of image via a spherical nearest neighbour interpolation scheme. On top of this, we have Frankenstein-ed the pcolormesh method to capture any non-quadilateral (or wrapping) data to allow plotting of data which might very well cross the dateline without a hitch.
I can put together some examples (sadly documentation is still lacking for cartopy) if interested.
Overall looks great. I left some inline comments. One example request would be something like the figures shown here: http://stackoverflow.com/questions/13633254/offset-polar-axes-grid-display-coordinate-display, i.e. a face-on view of the Galaxy. This isn't a native WCS format, so maybe it doesn't make sense to include as a |
I think the biggest remaining thing to decide relates to @mdboom's comment regarding the
how do people feel about this? I kind of like the idea of using
which can then be used, and everything done on it will work in Galactic coordinates. However, maybe that's a little confusing, because by default, all ticks and labels would be hidden (since maybe they just requested to overplot some polygons, but they don't care about getting a coordinate overlay). Maybe better to use the
Any thoughts? Finally, the only other tricky bit is to do with labels, in that there are tick and axis labels. However, maybe we can simplify by saying that any reference to labels for the coordinate objects is to do with tick labels, and we don't actually touch anything to do with axis labels, since the standard API is fine for that? |
I think I prefer the |
I decided to switch to For now, I decided to leave the slightly different methods for axis and tick labels, because I think it makes it much more readable, even if slightly inconsistent with matplotlib. Finally, I changed the section for multi-dimensional WCS so that this is now something the I think that concludes the comments I had to address - I will send one last email to the mailing list before merging this, if there are no objections. |
The latest rendered version can be seen here: https://github.com/astrofrog/astropy-api/blob/7fba6015426e1604440b85f2cf0964ff7b6c9d35/wcs/wcs_api.md |
If we want to do something like Most easy way is to have an Axes method that returns a new transform, but again this introduces a new method, and not very elegant I guess. Personally, I am not a big fan of using this api. I think this, in some sense, abuses the transform keyword ("abuse" maybe too strong). But if others agree to go this way, that's completely fine with me. But I think we should discuss how to implement this! To me, if we have different transform (transData, more exactly), it is natural to introduce a new axes. This is somewhat similar to And, I think the api like below ax_gal = ax.frame('gal') have good advantage over the current api due to similar reasons that already mentioned by @astrofrog. |
an additional argument for the WCS object. | ||
|
||
import matplotlib.pyplot as plt | ||
from astropy.wcs import WCS, WCSAxes |
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.
Are we going to put WCSAxes inside the wcs module? They are quite different in nature, and I am not sure if this is a best idea, although this not critical.
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 they belong together because one is really just a graphical representation of the other - and WCS
has to be able to return a WCSAxes
instance from the _as_mpl_axes
method. In fact, users shouldn't even have to invoke WCSAxes
directly, since they can do projection=WCS(...)
. But I'm interested to hear your opinion about why you feel they should belong in a different module?
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.
Well, I think my view is that wcs.WCS
is one implementation of the world coordinate system, and WCSAxes
is one implementation of the graphical representation of the world coordinate system. Anyhow, this is nothing important.
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 I'm with @leejjoon on this one, but even further ;) astropy.wcs.WCS
is one implementation (wcslib
) of one representation (FITS WCS
) of WCS. In fact, once we can make such a change, I'd almost want to rename it to fits_wcs
to help distinguish it from the more general WCS that are sure to come... I think I'd prefer they were independent models. I can envision this plotting work being extended in the future to have different transformation backends, the first being astropy.wcs
, but many to come in the future.
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.
Right, I should have said that when I was talking about the WCS
class, it doesn't have to be FITS WCS only. In my mind, astropy.wcs
was intended to contain any WCS system in the long term, so that's why I see WCSAxes living there. However, if you still think it would be better to have it elsewhere, we could have an Astropy sub-package dedicated to plotting, e.g. astropy.plotting
or astropy.graphics
where other Astronomy-specific visualizations could live? I don't think this is an important point for now though, as I think we'll start implementing it as an affiliated package anyway until it's ready for inclusion, at which point we can decide where it should live (and by then, maybe we'll have made a decision concerning the generalized WCS).
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 agree with @mdboom and @leejjoon that that location implies a tie to the WCS
object rather than to a future "generalized" WCS. The question is where the gWCS ends up - if we end up creating astropy.gwcs
or something, it's a problem that WCSAxes
is in wcs
. But @astrofrog is right that we may want to end up putting the generalized WCS in astropy.wcs
anyway.
So if we are sure we don't want the future gWCS stuff in wcs
, than it makes sense for WCSAxes
to be elsewhere. But if there's a chance of it being in wcs
, I agree with @astrofrog that it's more expedient and understadable to put it here for now.
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.
The thing is that the recommended usage for this functionality is actually going to be
ax = fig.add_subplot(1, 1, 1, projection=wcs.WCS('image.fits'))
so it's actually the WCS class that's passed to matplotlib, and this calls WCS._as_mpl_axes
which has to return an Axes class, so doesn't it make sense for the axes class to live alongside the WCS class? The point is that I think the WCSAxes
class may even be 'private' and never accessible to the user, which is why I'm not so keen on creating a new sub-package for it.
Since it'll be a private class anyway, then it doesn't matter where it lives so we can just put it alongside WCS
.
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.
Well, I don't see why we would make it private. It will end up user-facing because if you do pyplot.gca()
or similar that's what object you will get. So I think it should still be treated as a "public" class in terms of API decisions. (Also, I could certainly imagine someone wanting to subclass it in some circumstances)
Don't get me wrong, I agree with you that wcs
is a sensible place right now... I just don't think we want to treat it as "subordinate" to the current WCS
given that we'd rather it eventually be tied more closely to the generalized WCS. (I wish there was an emoji 🤷)
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.
You are right that the class needs to be accessible directly too.
For now, I'll develop it alongside the current WCS, but of course we can rethink this when we come up with the generalized WCS.
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 the tick and label related API is a bit ambiguous. In one example, I see glon.set_ticklabel_position('t')
glon.set_ticklabel_color('green') Are you suggesting that we implement methods like these for all (or most) properties, e.g., fontsize, alpha, etc? Maybe one option is to just adopt |
@leejjoon - ah, I did not know about |
Thinking about this more, I agree with @leejjoon that using
Furthermore, since we can show and hide the coordinate grid with:
then it makes sense that one could simply do
to show an overlay, so we don't need a new method for that. |
Both approaches have pros and cons. So, here is my suggestion. I think the first step toward this is to have an API such as tr = ax.get_sky_transform("fk5") The returned transform will be essentially = "transform from fk5 to pixel coordinate of ax" + ax.transData. When the argument is a WCS instance tr = ax.get_sky_transform(wcs.WCS("another.fits")) then the returned transform is = "transform from pixel coordinate of 'another.fits' to the pixel coordinate of ax" + ax.transData. (1) Given that we have this method, we will be able to do something like below. tr_fk5 = ax.get_sky_transform("fk5")
ax.plot(pos_ra, pos_dec, "o-", transform=tr_fk5) (2) Once Matplotlib is modified to support such a usage like (3) With a method like So, we first implement (1). We then implement (3) for advanced users. And then implement (2) when matplotlib is ready. How others think? Anyhow, the name of |
@leejjoon - I like this approach - very clean! I wonder whether |
With the current API I proposed, |
@leejjoon - ah I see, you don't want to tie the WCS class to Matplotlib. In that case, what you suggested makes sense. |
@leejjoon - I've implemented your suggestion, which I really like, although I renamed it |
No additional comments from me. |
identity, meaning that the WCSAxes object should then act like a normal Axes | ||
object. | ||
|
||
We can also provide pyplot-style initialization with the ``projection`` keyword: |
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.
"pyplot-style" to mean means the functional paradigm of using the pyplot
functions, like matplotlib.pyplot.axes
and matplotlib.pyplot.subplot
. But below it looks like you're talking about the OO-paradigm of doing fig.add_suplot(...)
. Do you intend to add functions to support the functional interface?
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.
My understanding is that once we implement WCS. _as_mpl_axes
, we can call projection=WCS(...)
in both the functional (matlab-style) interface, as well as the OO-like and full-OO API.
@mdboom - can you confirm whether this is the case?
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.
By the way, if this is the case, I will clarify this in the document.
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 clarified this in the document.
Aside from my comment above (and my response to one of the old comments further up, which may not require any action), this seems good to me! |
I will now merge this - thank you to everyone for your feedback! Now comes the hard part, actually implementing it :) I will start co-ordinating this at some point over the next few weeks. If you have any suggestions for improvements, please open a new issue, or even better, a pull request! |
A number of packages, such as pywcsgrid2, APLpy, and kapteyn, have developed ways of making plots in world coordinate systems (WCS) in Matplotlib, and since this is a common issue in Astronomy, we are interested in implementing the general problem of plotting WCS axes in the Astropy core, so that we can avoid duplicating this in other packages. The present document describes the proposed API.
The aim is not to provide the simplest possible user-friendly API to the user, but to design a flexible API consistent with the matplotlib API upon which user-friendly tools can be developed.
This API was written by @astrofrog and @leejjoon, and benefited from feedback from @ebressert and @ChrisBeaumont. It is written in markdown, since we wanted to include images. The latest rendered version of the API document (with images) can be seen here:
https://github.com/astrofrog/astropy-api/blob/7fba6015426e1604440b85f2cf0964ff7b6c9d35/wcs/wcs_api.md
At this time, we would be very interested in getting feedback from other developers (or users) regarding this API. In some places, the API may not be optimal, so please let us know if you have any suggestions! Also, if you see anything missing, please let us know (and offer suggestions if possible!).
(edited to update link)