-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
allow passing coordinate names as x and y to plot methods #608
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
allow passing coordinate names as x and y to plot methods #608
Conversation
@@ -45,6 +45,11 @@ def _infer_xy_labels(plotfunc, darray, x, y): | |||
|
|||
darray is a 2 dimensional data array. | |||
""" | |||
|
|||
if all(v in darray.coords for v in {x, y}): |
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.
Set comprehensions don't work in Python 2.6 (which we still support, alas).
Maybe instead of the complex fallback logic to handle specifying x and/or y, we should rewrite this to only handle two cases:
Something like this... def _infer_xy_labels(plotfunc, darray, x, y):
if x is None and y is None:
if darray.ndim != 2:
raise ValueError('must be 2d')
x, y = darray.dims
elif x is None or y is None:
raise ValueError('cannot supply only one of x and y')
elif any(k not in darray.coords for k in (x, y)):
raise ValueError('x and y must be coordinate variables')
return x, y |
This is exactly where I was headed. |
elif x is None or y is None: | ||
raise ValueError('cannot supply only one of x and y') | ||
elif any(k not in darray.coords for k in (x, y)): | ||
raise ValueError('x and y must be coordinate variables %s' % darray.coords) |
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.
Just print the coord names.
I think this is ready for a review. This fixes #611 and allows us to pass 2d coordinate names into plotting methods. # Fixed FacetGrid labels
fg = da.plot.pcolormesh(col='time', col_wrap=4) # 2d coord names
ax = plt.axes(projection=Rasm())
ds.frac.plot.pcolormesh(x='xc', y='yc', transform=cartopy.crs.PlateCarree()) |
@@ -511,6 +469,11 @@ def imshow(x, y, z, ax, **kwargs): | |||
The pixels are centered on the coordinates values. Ie, if the coordinate | |||
value is 3.2 then the pixels for those coordinates will be centered on 3.2. | |||
""" | |||
|
|||
if x.ndim != 1 or y.ndim != 1: | |||
raise ValueError('Imshow requires 1D coordinates, try using ' |
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.
nit: I wouldn't capitalize imshow
because it's a python identifier
Can we make it so the generic |
How about making the default |
👍 |
While you're at it -- maybe the default |
so you would have a DataArray of shape (1, 5) be plotted via pcolormesh (by default)? |
Either that, or we should extract the On the other hand, it's not so terrible to encourage people use to explicit plot method in applications instead of the generic |
I started to go down the road of removing the squeeze you were talking about but |
OK, works for me :) On Wed, Oct 7, 2015 at 2:40 PM, Joe Hamman [email protected] wrote:
|
So, are we okay with the default 2D plot being I think @clarkfitzg may have an objection. |
Originally, we only had imshow and contourf. We picked imshow as the default because it shows individual pixels. But pcolormesh also meets that criteria, and has the convenience of also working on non-evenly spaced grid. The only reason to possibly not pick pcolormesh is that it has a different convention from imshow in terms of the image orientation (top-left vs bottom-left origin). Given that xray is largely used for data with meaningful coordinate labels (rather than images), I think it's probably a better choice to use the bottom-left origin as a default. It composing better, for example, with 1D plots. |
@@ -657,7 +707,7 @@ def test_imshow_called(self): | |||
self.assertTrue(self.imshow_called(self.darray.plot.imshow)) | |||
|
|||
def test_xy_pixel_centered(self): | |||
self.darray.plot.imshow() | |||
self.darray.plot.imshow(yincrease=None) |
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.
why does this become necessary?
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.
good catch. At one point this test was failing for me so I was playing around, trying to understand why. Forgot to roll back that change. Fixed in d793e3a.
The problem, however, is time! How long does it take to plot and render with the two?
|
With a 1000 x 1000 grid I'm seeing 10 ms for |
This is a good point. For me, its not a deal breaker though and the benefits of using pcolormesh all the time (simpler code, broad applicability) are worth the wait. |
I think pcolormesh is slightly more likely to yield correct and consistent results for plotting different types of data than our current hybrid of imshow/contourf. In my experience, pcolormesh also works better with cartopy. For default behavior, I would lean towards being correct rather than fast. Of course, waiting ~0.5 seconds per plot does start to add up when you're faceting, but it's not much more trouble to type .imshow. On Wed, Oct 7, 2015 at 7:04 PM, Joe Hamman [email protected]
|
Simplification of the logic is a good thing, so I'm fine with this. We should call out the performance difference in the docs, maybe both in the main 2d plot section and the faceting section. |
I will add a bit to the docs. |
I have updated the docs with a note about speed. I also updated the default behavior of |
I'm conflicted about what the default @jhamman could you please explain your reasoning a little more for why we need this switch? Do you have a comprehensive sense of the ways that people organize their data cubes? With climate data, it does seem pretty common that datasets have decreasing latitude values (e.g., 90, 80, 70, ...). This works well with the default behavior of |
Yes, that was my motivation. Basically, switching to pcolormesh requires us to do this - the alternative is to add I wouldn't say I have a comprehensive sense of how everyone orders their tick labels. In the earth sciences, which is my area of study, I'd say 95% of plots use the standard increasing y notation. The most common use cases for a decreasing y coordinate would probably be depth below the surface (where depth is increasingly positive). One nice thing about setting |
I think it's fine to have |
I'm OK with changing the default to |
I'm fine with adding a global option, but yes, we should wait until someone asks for that feature. |
LGTM |
allow passing coordinate names as x and y to plot methods
Thanks @shoyer and @clarkfitzg for the input on this. Funny how a small change can sometime unravel into a much larger issue. |
thanks @jhamman for making it work! |
This allows us to use coordinate names as plotting arguments for
x
andy
.I have a test for this that isn't quite working yet.