Conversation
for more information, see https://pre-commit.ci
xarray/plot/utils.py
Outdated
| if levels is not None: | ||
| cmap, newnorm = _build_discrete_cmap(cmap, levels, extend, filled) | ||
| norm = newnorm if norm is None else norm | ||
| if isinstance(norm, mpl.colors.BoundaryNorm): |
There was a problem hiding this comment.
I think there is some incompatibility between "levels" and "BoundaryNorm" - you really shouldn't use both and there should be no need to discretize the colormap if using a BoundaryNorm; conversely, there is no need to use a BoundaryNorm or discretize the colormap if using "levels" in a contour plot.
I'm not fully familiar with the xarray pcolormesh API, but I would raise if the user passes both BoundaryNorm and "levels", as having incompatible or duplicated specifications of the color boundaries, and I wouldn't do anything if a BoundaryNorm is passed in. If you want discrete colormaps with levels, but a non-linear norm, I think _build_discrete_cmap is fine.
There was a problem hiding this comment.
Thanks for your valuable feedback. I see your point and I agree, since the "levels" can be derived from BoundaryNorm.vmin, BoundaryNorm.vmax and BoundaryNorm.N. I also considered your alternative suggestion and issues #4061 and Deltares/xugrid#49 are indeed fixed by supplying levels instead of norm to ds.plot().
However, if it is not beneficial to supply norm=BoundaryNorm to ds.plot(), I would say it should raise an error instead of being incorrectly catched in an if-statement. Would you agree?
There was a problem hiding this comment.
@jklymak: could you let me know if you agree. If so, I will make a separate issue and close this PR.
There was a problem hiding this comment.
Assuming we are talking about pcolormesh, Matplotlib has no levels kwarg, so it's a bit strange for xarray to have grown one. I would not prohibit the Matplotlib way of doing this (BoundaryNorm). If you want to keep the convenience of levels, then I would just error if both levels and BoundaryNorm are passed.
In either case, I'm not clear why folks are using levels at all for a pcolormesh, but that is just me ;-)
There was a problem hiding this comment.
There was a problem hiding this comment.
If there is a BoundaryNorm already, I'm not following why you are wanting to build a discrete cmap - the norm will discretize the underlying cmap for you...
There was a problem hiding this comment.
@jklymak You can ignore the PR I did, this is a workaround for the issue in #4061, but as I have learned from you it is not an improvement for xarray. The issue I am referring to provides the norm argument upon plotting, but it behaves different than the matplotlib. I do not understand the xarray plotting code good enough to provide a proper fix for this issue since it is a bit complex. As far as I understand, this PR can be closed without merging, but the issue is not fixed yet. Maybe you can suggest a proper fix for the code in the issue? You seem to understand how the plotting methods should behave.
There was a problem hiding this comment.
I think this PR is very close, just you went the opposite way with it. I think it should actually be
if (levels is not None) and (not isinstance(norm, mpl.colors.BoundaryNorm)):
and not create a colormap if BoundaryNorm is being used....
|
Thanks @jklymak, that makes sense I guess. I have committed the changes again. Could you check if it can be merged? |
headtr1ck
left a comment
headtr1ck
left a comment
There was a problem hiding this comment.
You can add an entry in whats-new.
…m_matching_cmap_levels()
for more information, see https://pre-commit.ci
…m_matching_cmap_levels(), again
|
@headtr1ck the proposed changes were pushed agian. I think most should be fixed now. |
for more information, see https://pre-commit.ci
Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>
|
Thanks for this PR! |
* upstream/main: Save groupby codes after factorizing, pass to flox (pydata#7206) [skip-ci] Add compute to groupby benchmarks (pydata#7690) Delete built-in cfgrib backend (pydata#7670) Added a pronunciation guide to the word Xarray in the README.MD fil… (pydata#7677) boundarynorm fix (pydata#7553) Fix lazy negative slice rewriting. (pydata#7586) [pre-commit.ci] pre-commit autoupdate (pydata#7687) Adjust sidebar font colors (pydata#7674) Bump pypa/gh-action-pypi-publish from 1.8.1 to 1.8.3 (pydata#7682) Raise PermissionError when insufficient permissions (pydata#7629)
4 participants
whats-new.rstapi.rst>> not applicable