-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 x,y kwargs for plot.line(). #1926
Conversation
xarray/plot/plot.py
Outdated
_ensure_plottable(x) | ||
else: | ||
if x is not None and x is not darray.name: | ||
raise ValueError('Cannot make a line plot with x=%r, y=%r and hue=%r' |
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.
E501 line too long (85 > 79 characters)
e57b452
to
87bb820
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.
Thanks @dcherian ! I have a couple of comments and a general question: do we want to add the control via variable name? I think it makes things more confusing for little added value (names have no control on other plots such as 2D plots).
Otherwise this looks quite good!
doc/plotting.rst
Outdated
@@ -197,6 +197,16 @@ It is required to explicitly specify either | |||
Thus, we could have made the previous plot by specifying ``hue='lat'`` instead of ``x='time'``. | |||
If required, the automatic legend can be turned off using ``add_legend=False``. | |||
|
|||
Data along x-axis |
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.
"Rotated line plots" as title? Just a suggestion
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.
Rotation makes me think of rotating the lines by an angle. How about "Coordinate along y-axis"?
doc/plotting.rst
Outdated
Data along x-axis | ||
~~~~~~~~~~~~~~~~~ | ||
|
||
It is also possible to make line plots such that the data are on the x-axis and a co-ordinate is on the y-axis. This can be done by specifying the ``x`` and ``y`` keyword arguments. |
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.
"co-ordinate" -> coordinate
doc/whats-new.rst
Outdated
@@ -97,6 +97,8 @@ Enhancements | |||
encoding/decoding of datetimes with non-standard calendars without the | |||
netCDF4 dependency (:issue:`1084`). | |||
By `Joe Hamman <https://github.com/jhamman>`_. | |||
- :py:func:`~plot.line()` learned to make plots with data on x-axis if so specified. |
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.
add (:issue:`575`)
xarray/plot/plot.py
Outdated
1D and 2D DataArrays: Coordinate for x axis. | ||
y : string, optional | ||
1D DataArray: Can be coordinate name or DataArray.name | ||
2D DataArray: Coordinate for y axis. |
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'm not sure this is really helping. I'd prefer to stick to the simple Coordinate for x/y axis.
xarray/plot/plot.py
Outdated
if x is not None and y is not None and x == y: | ||
raise ValueError('Cannot make a plot with x=%r and y=%r' % (x, y)) | ||
|
||
if (x is None and y is None) or x == dim or y is darray.name: |
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.
y is darray.name
-> y == darray.name
xarray/plot/plot.py
Outdated
xlabel = dim | ||
ylabel = darray.name | ||
|
||
elif y == dim or x is darray.name: |
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.
here too
@fmaussion when I added the It would also mean that @shoyer Thoughts? EDIT: @fmaussion Do you mean |
Yes, I meant that there should be one and only way to specify which axis should do what: my understanding of your current implementation is that there are two ways to reach the same goal: either by (i) specifying the name of the variable to the axis you want to plot it onto, or (ii) by specifying the name of the coordinate you want to use for the axis. Since (ii) is the default and only option for 1D and 2D plots until now, I just wondered if (i) is very necessary. (or maybe I missed something) |
My main thought is that this API would feel much more natural on a Dataset object, alongside a That said, I suppose this could still be useful and I don't think it's harmful to expand the API here. It does feel a little strange that if you had a DataArray with non-dimension coordinates you could make a plot without including any of the DataArray values, e.g., |
Good points. I'll change it so that only one of
It's very useful in oceanography (and meteorology) where we want to make plots of physical quantities like temperature against depth; it is much more natural to see depth on the y-axis than the x-axis. This is the original usecase @rabernat mention in #575. The Dataset API is a more involved undertaking that I'm not confident of executing at this point. |
xarray/plot/plot.py
Outdated
add_legend = kwargs.pop('add_legend', True) | ||
|
||
ax = get_axis(figsize, size, aspect, ax) | ||
|
||
error_msg = 'must be either None or %r' % (' or '.join(darray.dims)) | ||
|
||
if x not in [None, *darray.dims]: |
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.
E999 SyntaxError: invalid syntax
xarray/plot/plot.py
Outdated
add_legend = kwargs.pop('add_legend', True) | ||
|
||
ax = get_axis(figsize, size, aspect, ax) | ||
|
||
error_msg = 'must be either None or one of %r' % list(darray.dims) | ||
|
||
if x not in [None, *darray.dims]: |
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.
E999 SyntaxError: invalid syntax
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 don't know what's wrong here. It runs locally; the tests pass and flake8 goes through plot.py without errors. Is it a python2 vs python3 thing?
Yes, this is not valid syntax on Python 2.
…On Wed, Feb 21, 2018 at 9:53 AM Deepak Cherian ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In xarray/plot/plot.py
<#1926 (comment)>:
> add_legend = kwargs.pop('add_legend', True)
ax = get_axis(figsize, size, aspect, ax)
+ error_msg = 'must be either None or one of %r' % list(darray.dims)
+
+ if x not in [None, *darray.dims]:
I don't know what's wrong here. It runs locally; the tests pass and flake8
goes through plot.py without errors. Is it a python2 vs python3 thing?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1926 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABKS1pXVXGKhgmkR8yP3epLPvqZCXNipks5tXFgpgaJpZM4SLdeS>
.
|
xarray/plot/plot.py
Outdated
error_msg = ('must be either None or one of ({0:s})' | ||
.format(', '.join(['\''+dd+'\'' for dd in darray.dims]))) | ||
|
||
if x not in [None, *darray.dims]: |
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.
E999 SyntaxError: invalid syntax
9e75869
to
fabdc04
Compare
xarray/tests/test_plot.py
Outdated
|
||
for aa, (x, y) in enumerate(xy): | ||
da.plot(x=x, y=y, ax=ax.flat[aa]) | ||
ax.flat[aa].set_title('x=' + str(x) + ' | '+'y='+str(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.
E226 missing whitespace around arithmetic operator
xarray/plot/plot.py
Outdated
add_legend = kwargs.pop('add_legend', True) | ||
|
||
ax = get_axis(figsize, size, aspect, ax) | ||
|
||
error_msg = ('must be either None or one of ({0:s})' | ||
.format(', '.join(['\'' + dd + '\'' for dd in darray.dims]))) |
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 use repr(dd)
here instead of '\'' + dd + '\''
xarray/plot/plot.py
Outdated
raise ValueError('y ' + error_msg) | ||
|
||
if x is not None and y is not None: | ||
raise ValueError('You cannot specify both x and y kwargs.') |
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 should probably be qualified by "for line plots"
Supports both 1D and 2D DataArrays as input. Change variable names to make code clearer: 1. set xplt, yplt to be values that are passed to ax.plot() 2. xlabel, ylabel are axes labels 3. xdim, ydim are dimension names
01153d2
to
ab1431f
Compare
ab1431f
to
3cd49cb
Compare
Done and rebased on master. |
Coordinate for which you want multiple lines plotted | ||
(2D DataArrays only). | ||
x, y : string, optional | ||
Coordinates for x, y axis. Only one of these may be specified. |
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 should probably say something like "The other coordinate plots values from the DataArray on which this plot method is called."
xarray/tests/test_plot.py
Outdated
|
||
for aa, (x, y) in enumerate(xy): | ||
da.plot(x=x, y=y, ax=ax.flat[aa]) | ||
ax.flat[aa].set_title('x=' + str(x) + ' | ' + 'y=' + str(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.
Is there any point to this line?
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.
No. I copied the test over from my debugging plots. I'll remove it (I assume you mean the set_title
bit).
xarray/tests/test_plot.py
Outdated
[None, 'z'], | ||
['z', None]] | ||
|
||
f, ax = plt.subplots(2, 4) |
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 only have two cases now, so no need for 2x4 subplots.
@fmaussion any further concerns here? This looks good to me. |
Thanks @dcherian ! This is a nice feature |
whats-new.rst
for all changes andapi.rst
for new APIDescription
plot.line
now supports both 1D and 2D DataArrays as input. I've changed some variable names to make code clearer:xplt
,yplt
to be values that are passed toax.plot()
xlabel
,ylabel
are axes labelsxdim
,ydim
are dimension namesExample
This code
yields
Feedback requested
Should I refactor out the kwarg checking?