-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
plot.line(): Draw multiple lines for 2D DataArrays. #1785
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
Conversation
a9b5d76
to
4572fd4
Compare
4572fd4
to
28ae492
Compare
Looks quite good! Thanks. Sorry to be annoying ;), but there should be tests for the features you document:
Currently, choosing another dim for the x axis and the legend kwargs are not tested. |
0f31b95
to
61d5226
Compare
@fmaussion I've added more tests. Let me know if you'd like more changes. |
61d5226
to
46b45c6
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.
Looks good to me, thanks!
Is there a keyword argument name that it would make sense to use for the dimension that is repeated into multiple lines? Maybe I would like there to be a fully explicit way to make these plots. |
@fmaussion I've added one more commit. If Extended an existing test |
@shoyer |
For arrays shaped as (100000, 2), this will plot 2 lines instead of 100000!
e2a5c65
to
7d3cf6c
Compare
I am very nervous about automated heuristics for choosing behavior. I would much rather we raise an error message in cases like this, rather than guessing. (The problem is that heuristics can make it very hard to predict/understand how code will work without trying it.) |
I've added support for a
Well, this behaviour is analogous to automatically choosing The current behaviour for a 10000x3 array is to plot 10000 lines which is bad. I'm OK with adding an error message but strongly feel that choosing to plot 3 lines (i.e. always smallest number of lines) is a good default. Re:error, would that be a message stating that either |
We use the order of the dimensions on the array for choosing how to plot it. The analogous behavior would be to always plot longer dimension along the x-axis, which isn't what we do.
I agree that users probably rarely want 10,000 lines :). That's a good reason to require an explicit choice here. The problem are edge cases like a 5x6 array. Do you want 5 lines of 6 points each or 6 lines of 5 points each? If we make the heuristic depend on the size of the array, then it will give very hard to understand what happens when the array shape changes slightly.
Yes, that sounds right. |
Added an error if provided 'x' is not a dimension for 1D input (to mirror _infer_xy_labels behaviour for 2D inputs) Updated tests.
@shoyer , @fmaussion : Updated to require either |
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.
One minor point -- otherwise looks good to me, thanks!
xarray/plot/plot.py
Outdated
if ndims == 1: | ||
xlabel, = darray.dims | ||
if x is not None and xlabel != x: | ||
raise ValueError('Input does not have specified dimension ' + x) |
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.
please use string formatting instead so this still works even if the dimensions is not a string, e.g.,
raise ValueError('Input does not have specified dimension {!r}'.format(x))
(Using non-strings as dimension names is rare but we still try to support it when feasible.)
@shoyer Used your suggested change. |
xarray/plot/plot.py
Outdated
@@ -208,7 +208,8 @@ def line(darray, *args, **kwargs): | |||
if ndims == 1: | |||
xlabel, = darray.dims | |||
if x is not None and xlabel != x: | |||
raise ValueError('Input does not have specified dimension ' + x) | |||
raise ValueError('Input does not have specified dimension' | |||
+ ' {!r}'.format(x)) |
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 for future reference: you can skip the +
character and rely on implicit string concatenation.
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.
Didn't know about that. Thanks.
Thanks! |
I merged this, but it might also be nice to add an example (or at least brief mention) to the narrative docs for 1D plotting. See: |
git diff upstream/master **/*py | flake8 --diff
(remove if you did not edit any Python files)whats-new.rst
for all changes andapi.rst
for new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)Adds support for plotting multiple lines if
plot.line()
is provided with a 2D dataarray.kwarg
x
lets you specify co-ordinate for x-axis.Example: