-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add DivergingNorm (again, again, again) #12419
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
Actually in the light of day I’m not going to champion this. If someone tried to publish in my field with negative anomalies on a different colour-to-data ratio than positive anomalies they would be in for a drubbing. Maybe there are applications that I don’t know about where this feature would be desirable and legitimate so feel free to reopen this or one of its predecessors. But, I don’t know the use case for encouraging this sort of data mapping by default. |
The use of such Norm is clearly that you may work with one single colormap and adapt the norm to the data in use. The following image is taken from this stackoverflow question where the idea is to define a "sealevel", such that the turquoise part of the colormap is always at 0 elevation. The alternative to this approach would be to create a new colormap for each individual dataset, which is surely undesireable. |
Ahh. Thanks the sea level one is a great example where this is ok. I’ll change the examples and tests to that 😉 |
There is by the way a part about this in the docs: https://matplotlib.org/users/colormapnorms.html#custom-normalization-two-linear-ranges which tells that "This may appear soon as colors.OffsetNorm()" and it has the warning and the use case mentionned:
|
Ha ha, I wrote that 😉 |
In the current version that example has somehow become useless, as it suddenly uses data that is symmetric about 0 anyways. It also lost the warning. There is also an error in the BoundaryNorm section. I suppose that was edited in a hurry and would need to be refurbished anyways. |
lib/matplotlib/tests/test_colors.py
Outdated
import numpy as np | ||
import pytest | ||
|
||
from numpy.testing import assert_array_equal, assert_array_almost_equal | ||
from numpy.testing import (assert_array_equal, assert_array_almost_equal, | ||
assert_equal) |
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 think we've generally moved away from using assert_equal
. Especially because you're just comparing single values, just assert that the two things are equal.
lib/matplotlib/colors.py
Outdated
vmax = np.ma.max(A) | ||
|
||
if vmin > vmax: | ||
raise ValueError('vmin must be less than vmax') |
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.
less than or equal (or change condition)?
db8c3de
to
38cb3dc
Compare
lib/matplotlib/colors.py
Outdated
raise ValueError('vmin, vcenter, and vmax must ' | ||
'increment monotonically for DivergingNorm ' | ||
raise ValueError('vmin, vcenter, and vmax must increment ' | ||
'monotonically for DivergingNorm ' |
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.
minor point, but would it be more clear to say?
DivergingNorm requires that
vmin <vcenter < vmax
28c659b
to
323d41f
Compare
@pharshalp this now doesn't clip the values outside vmin, vmax unless clip is True... |
@jklymak Thanks! Just checked it using a small example and can confirm that it works! import numpy as np
import matplotlib.pyplot as plt
import matplotlib.colors as mcolors
x = np.linspace(-2, 7)
y = np.linspace(-1*np.pi, np.pi)
X, Y = np.meshgrid(x, y)
Z = x * np.sin(Y)**2
fig, (ax1, ax2) = plt.subplots(ncols=2)
cmap = plt.cm.coolwarm
cmap.set_over('orange')
cmap.set_under('steelblue')
norm = mcolors.DivergingNorm(vmin=-1, vcenter=0, vmax=6)
img1 = ax1.pcolormesh(Z, cmap=cmap, norm=None)
cbar1 = fig.colorbar(img1, ax=ax1, extend='neither')
img2 = ax2.pcolormesh(Z, cmap=cmap, norm=norm)
cbar2 = fig.colorbar(img2, ax=ax2, extend='both')
plt.show() |
Any idea why the two new norms do not appear in the "classes" list in the docs? |
py 3.5 errors are due to slight differences in whether interpolation is under or over in numpy (i.e. does 1.0 interpolate into [0. 1.] as an over value or not - 3.6/3.7 say not, 3.5 says yes). What is the plan for 3.1 wrt py 3.5? Discuss in #12358 |
9fe878e
to
3d6c3c6
Compare
|
||
bunique = b | ||
yunique = self._y | ||
if b[0] == b[1]: |
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 see how the previous code was doing linear interpolation by hand, but where do these "equal first and second point / equal last and nexttolast point" cases come from?
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.
Fixed comment:
bunique = b
yunique = self._y
# trim extra b values at beginning and end if they are
# not unique. These are here for extended colorbars, and are not
# wanted for the interpolation.
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 quite understand this. Souldn't you trim all but one duplicate values?
The current code does b = [0, 0, 0, 1, 2] --> bunique = [0, 0, 1, 2]
. Is that intended?
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.
That should never happen - if it does, just as well that an error trips. b
are the boundaries, passed through the norm should be monotonic, except for, possibly, the first and last values if there are an "extend" either at max or min.
cmap=terrain_map,) | ||
ax.set_xlabel('Lon $[^o E]$') | ||
ax.set_ylabel('Lat $[^o N]$') | ||
ax.set_aspect(1 / np.cos(np.deg2rad(49))) |
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 guess that comes from Mercator? kind of obscure if that's not your field, but probably not a big deal.
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.
Added explanation: This is just a local "projection" centered at 49 degrees taking into account how much closer the longitude lines are at that lattitude than at the equator and then assuming the lines of longitude are parallel. Not very sophisticated 😉
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.
Only minor nitpicks left, nothing blocking the merge.
Thanks @anntzer. Reasonable nitpicks that I’ll fix today. EDIT: Fixed! |
1c81602
to
31785c2
Compare
Borrows heavily from @Tillsen's solution found on StackOverflow here: http://goo.gl/RPXMYB Used with his permission dicussesd on Github here: https://github.com/matplotlib/matplotlib/pull/3858` TST: add tests for DivergingNorm DOC: add to colors_api.rst DOC: add tutorial FIX: fix extend=both DOC: add new example
assert norm.scaled() is True | ||
|
||
|
||
def test_DivergingNorm_scaleout_center(): |
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 and the one below could use pytest.mark.parametrize
assert norm.vmin == -5 | ||
|
||
|
||
def test_DivergingNorm_Even(): |
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.
and these two as well
assert_array_equal(norm(vals), expected) | ||
|
||
|
||
def test_DivergingNorm_VminEqualsVcenter(): |
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.
and these fives
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.
If someone wants to parametrize these post-facto they can. I am not a fan of parameterized tests....
…419-on-v3.1.x Backport PR #12419 on branch v3.1.x (Add DivergingNorm (again, again, again))
PR Summary
Edit: see documentation here (and examples linked therein)
https://19203-1385122-gh.circle-artifacts.com/0/home/circleci/project/doc/build/html/api/_as_gen/matplotlib.colors.DivergingNorm.html#matplotlib.colors.DivergingNorm
This PR is re-invigorating #5054, which re-invigorated #4666, which re-invigorated #3858. Who knows, maybe it went back before that. Hat tip to @dopplershift, @OceanWolf , @jkseppan, @phobson who wrote this, not me. EDIT: forgot #5061....
It (re-) introduces
DivergingNorm
:This seems like a pretty straightforward and helpful enhancement, so not sure why this is the fourth PR.
OTOH, please note the todo-list is still active, this needs docs, whats new, and more thorough tests and I've not yet double checked the code. But throwing it out there in case any of the original authors want their PR back.Mostly ready to go, but no doubt some edge cases...
PR Checklist