-
-
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
allow multiindex levels in plots #3938
allow multiindex levels in plots #3938
Conversation
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 the doc enough or should I add something to plotting.rst
?
After this change the error messages are not entirely accurate any more.
- For the 2D plots seem to be not entirely correct but ok - I'd be fine to leave them, but maybe someone has a suggestion.
- For the 1D plot they become wrong (when the
da
has a MultiIndex), see below:
import xarray as xr
import numpy as np
da_1D = xr.DataArray(
np.arange(5),
dims="x",
coords=dict(a=("x", np.arange(5)), b=("x", np.arange(5, 10))),
)
da_1D = da_1D.set_index(x=["a", "b"])
da_1D.plot(x="not_valid")
returns
ValueError: x must be either None or one of ('x')
(but x="x"
does not work, you cannot specify a MultiIndex, only its levels). Should I
ValueError: x must be either None or one of ('a', 'b')
or
ValueError: x must be None, a dimension name, coordinate variable or coordinate level
I think this is ready for review. I simplified the error messages for the 1D case - let me know if this ok. |
This seems potentially confusing because I don't think any other xarray operation works with multiindex levels. Would it be more intuitive to allow plotting with multiindexes by converting the multiindex to a vector of strings and passing that to MPL? |
This should of course not lead to confusion, however, there are operations that work with levels, e.g. In my case I had Coordinates that looked something like this
and I wanted to do (Both "non-dimension coordinate" and "MultiIndex" have some issues, so depending on what I want to do I need one ore the other.) *which is why I only needed to change the valitity check for |
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 @mathause . I don't use MultiIndexes often so I didn't know that the individual levels are usable. This looks like a good idea to me. Minor suggestions below.
xarray/plot/plot.py
Outdated
raise ValueError("x " + error_msg) | ||
if ( | ||
x is not None | ||
and x not 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.
can we make valid_xy = set(darray.dims) + set(darray.coords) + set(darray._level_coords)
and then print those values in the error message. It would make the if conditions on x and y pretty clean too.
Could do the same below too.
Is it possible to refactor out the x
and y
checking?
👍 on adding something to |
This would be ready for review ;) |
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.
LGTM. Thanks @mathause. Also thanks for being patient here.
No problem. Thanks for the helpful feedback and your dedication to xarray! |
@dcherian do you think you could merge for me? |
Thanks @mathause! |
* upstream/master: Improve interp performance (pydata#4069) Auto chunk (pydata#4064) xr.cov() and xr.corr() (pydata#4089) allow multiindex levels in plots (pydata#3938) Fix bool weights (pydata#4075) fix dangerous default arguments (pydata#4006)
isort -rc . && black . && mypy . && flake8
whats-new.rst
for all changes andapi.rst
for new API