FIX: allow colorbar mappable norm to change and do right thing #13234

jklymak · 2019-01-20T00:59:45Z

PR Summary

import matplotlib.pyplot as plt
import matplotlib as mpl
import numpy as np

N = 100
X, Y = np.mgrid[0:3:complex(0, N), 0:2:complex(0, N)]
Z1 = (1 + np.sin(Y * 10.)) * X**(2.)
data = Z1

fig, ax = plt.subplots()
cax = ax.pcolormesh(data, cmap='RdBu_r', rasterized=True)
cbar = fig.colorbar(cax, ax=ax)
cax.set_norm(mpl.colors.PowerNorm(gamma=2, vmin=0.001, vmax=18))

Before

Note this colorbar is simply wrong...

After

Changing norm erases colorbar formatter and locator...

If the norm is changed, then any formatter and locator settings are also lost. This is new behaviour, but I think reasonable if the user has changed the norm. Here we illustrate this just by setting the ticks (which changes the Locator to FixedLocator). Note that when the norm is set the locator goes back to the automatic locator.

fig, ax = plt.subplots()
cax = ax.pcolormesh(data, cmap='RdBu_r', rasterized=True)
cbar = fig.colorbar(cax, ax=ax)

print('original', cbar.locator)
cbar.set_ticks([0, 10, 20])
print('first set ticks', cbar.locator)
cax.set_norm(mpl.colors.PowerNorm(gamma=2, vmin=0.001, vmax=18))
print('after norm changed', cbar.locator)
cbar.set_ticks(np.arange(0, 20, 2))
print('after ticks set second time', cbar.locator)
plt.show()

original <matplotlib.colorbar._ColorbarAutoLocator object at 0x127311be0>
first set ticks <matplotlib.ticker.FixedLocator object at 0x127351320>
after norm changed <matplotlib.colorbar._ColorbarAutoLocator object at 0x127311d68>
after ticks set second time <matplotlib.ticker.FixedLocator object at 0x1272f1080>

Next issue: Reset scale if norm changes...

The axis scale wasn't being reset if the norm changed, but it needs to be reset as well...

print('changing norm', cbar.ax.yaxis.get_scale())
pcm.set_norm(mcolors.LogNorm(vmin=1, vmax=100))
print('norm changed', cbar.ax.yaxis.get_scale())
pcm.set_norm(mcolors.Normalize(vmin=-20, vmax=20))
print('norm changed', cbar.ax.yaxis.get_scale())

Before

changing norm linear
norm changed linear
norm changed linear

After

changing norm linear
norm changed log
norm changed linear

I'm marking WIP until @LevN0 lets us know we squashed all the bugs...

Next Issue: format='%1.4e' now works...

See new test...

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

jklymak · 2019-01-20T16:32:06Z

Tests added. Note that this obviates the need for the user to call cbar.update_normal, at least for the cases I know about. ping @LevN0 and @efiring

efiring

Looks good.

timhoffm

Looks good. Can you please add a note to the docstring of set_norm that it will reset the formatter and locator?

NelleV · 2019-01-20T19:45:19Z

This looks good! Thanks a lot for the detailed explanation. It made reviewing much faster.
(I'm not approving yet, as Tim's comment still stands)

jklymak · 2019-01-20T20:12:39Z

OK, note added, though its a bit of a funky place to add a note about colorbar changes....

timhoffm · 2019-01-20T20:14:54Z

lib/matplotlib/cm.py

+        Notes
+        -----
+        If there are any colorbars using the mappable for this norm, setting
+        the norm of the mappable will reeset the norm, locator, and formatters


Typo.

Suggested change

the norm of the mappable will reeset the norm, locator, and formatters

the norm of the mappable will reset the norm, locator, and formatters

timhoffm · 2019-01-20T20:15:51Z

OK, note added, though its a bit of a funky place to add a note about colorbar changes....

True, but better than nothing and I don't have a better solution.

NelleV

LGTM!
I went ahead and fixed the small PEP8 error that made the tests fail.

NelleV · 2019-01-21T10:48:59Z

@jklymak I screwed up and didn't fix all the PEP8 from the github text editor… Do you mind fixing the rest (and maybe rebasing to squash the PEP8 fixes commits altogether?)

jklymak · 2019-01-21T14:52:04Z

Thanks for the review @NelleV. But this is still a WIP. I mucked around with so much it needs a thorough cleaning now. In particular for each colorbar draw_all is called three times which is unacceptable!

Stale! Sorry for the extra work!

jklymak · 2019-01-21T19:04:02Z

I think this is ready for now, but pinging @LevN0

Note that I decided we don't need to make locator and formatter properties, so I took that out to make things more straight forward...

timhoffm · 2019-01-21T20:35:12Z

lib/matplotlib/colorbar.py

@@ -387,30 +387,27 @@ def __init__(self, ax, cmap=None,
        self.outline = None
        self.patch = None
        self.dividers = None
+        # self.locator and self.formatter are properties...


this comment is not true anymore, I think.

timhoffm · 2019-01-21T20:36:00Z

lib/matplotlib/colorbar.py

@@ -449,14 +445,14 @@ def draw_all(self):
        if self.filled:
            self._add_solids(X, Y, C)

+    def set_norm(self, norm):
+        """
+        set the norm of the mappable associateed with this colorbar.


Suggested change

set the norm of the mappable associateed with this colorbar.

Set the norm of the mappable associated with this colorbar.

timhoffm · 2019-01-21T20:38:53Z

lib/matplotlib/colorbar.py

@@ -1105,6 +1131,7 @@ def on_mappable_changed(self, mappable):
        by :func:`colorbar_factory` and should not be called manually.

        """
+        _log.debug('mappable changed')


Suggested change

_log.debug('mappable changed')

_log.debug('colorbar mappable changed')

Giving slightly more context helps understanding the log output.

timhoffm · 2019-01-21T20:39:11Z

lib/matplotlib/colorbar.py

        """
+
+        _log.debug('update normal')


Suggested change

_log.debug('update normal')

_log.debug('colorbar update normal')

Giving slightly more context helps understanding the log output.

timhoffm · 2019-01-21T20:41:44Z

lib/matplotlib/tests/test_colorbar.py

+    assert isinstance(cbar.locator, _ColorbarLogLocator)
+    assert np.allclose(cbar.ax.yaxis.get_majorticklocs(),
+                       np.logspace(-8, 5, 14))
+    # note that that set_norm reemoves the FixedLocator...


Suggested change

# note that that set_norm reemoves the FixedLocator...

# note that set_norm removes the FixedLocator

timhoffm · 2019-01-21T20:43:11Z

lib/matplotlib/tests/test_colorbar.py

    assert np.isclose(cbar.vmin, z.min() * 1000)
    assert np.isclose(cbar.vmax, z.max() * 1000)


+def test_colorbar_format():
+    # make sure that format is passed properly....


Suggested change

# make sure that format is passed properly....

# make sure that format is passed properly

jklymak · 2019-01-21T21:48:00Z

Sorry for the WIP embargo and confusion that caused.

Most of the issues identified in #13228 have been addressed and this is ready for a second review.

NelleV · 2019-01-21T22:05:17Z

Is there a way to directly merge @timhoffm 's suggestion?

NelleV

I like @timhoffm 's suggestions. Good to go once they are merged!

jklymak · 2019-01-24T01:06:32Z

I think this is ready again:

fixed it so that changes to the norm vmin/vmax don't affect the formatter or locator, but actual changes to the norm do change the formatter/locator. Note modified test checks this behaviour.
no longer made ColorbarBase a subclass of ScalarMappable.
moved the deprecated methods that used to be from ScalarMappable into their own private class (mixin?)

Stale. Will review later.

jklymak · 2019-01-25T03:07:05Z

ping @efiring (@NelleV your review may be stale as well).

review is too old.

stale

efiring

It would be good to remove the reference to ScalarMappable inheritance in the tutorial:
https://matplotlib.org/tutorials/colors/colorbar_only.html#sphx-glr-tutorials-colors-colorbar-only-py

efiring · 2019-01-26T19:32:08Z

lib/matplotlib/colorbar.py

+    Private class to hold deprecated ColorbarBase methods that used to be
+    inhereted from ScalarMappable.
+    """
+    @cbook.deprecated("3.1", alternative="mappable.set_norm")


For alternative, did you mean ScalarMappable.set_norm here, as below?

Fixed both points! Thanks,

jklymak · 2019-01-28T02:22:07Z

This is technically an API change, but hopefully not a controversial one (the inheritance from ScalarMappable didn't really work properly anyways...)

jklymak · 2019-01-31T01:31:54Z

Did anyone else have time to look at this again? Changes from previous approvals are here

(squashed and rebased)

FIX: stop colorbar from being a ScalarMappable, and deprecate common fcns FIX: allow norm limit to change and not change user-set ticks and format FIX: moved old ScalarMappable methods to a private method DOC: fix deprecation and tutorial DOC: api note

LevN0 · 2019-02-01T01:05:15Z

Probably not addressed to me, but I looked at it again to confirm it still addresses the original issue, and it does.

NelleV

Looks good to me! I've added two minor comments on docstrings.

lib/matplotlib/colorbar.py

NelleV · 2019-02-02T19:56:13Z

Thanks!

lumberbot-app · 2019-02-02T19:56:26Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

$ git checkout v3.0.x
$ git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

$ git cherry-pick -m1 ce33de7afedbb2250a11aa102ba9f6996f6c259e

You will likely have some merge/cherry-pick conflict here, fix them and commit:

$ git commit -am 'Backport PR #13234: FIX: allow colorbar mappable norm to change and do right thing'

Push to a named branch :

git push YOURFORK v3.0.x:auto-backport-of-pr-13234-on-v3.0.x

Create a PR against branch v3.0.x, I would have named this PR:

"Backport PR #13234 on branch v3.0.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

…anged FIX: allow colorbar mappable norm to change and do right thing

The dummy parent class introduced in matplotlib#13234 had a `set_clim` with a different signature to the version in `ScalarMappable`. This caused code which called it and passed both arguments to fail, rather than simply no-op.

jklymak added status: work in progress status: needs tests labels Jan 20, 2019

jklymak force-pushed the fix-colorbar-norm-changed branch from 0300161 to 2af5f43 Compare January 20, 2019 05:32

jklymak added this to the v3.0.3 milestone Jan 20, 2019

jklymak removed status: needs tests status: work in progress labels Jan 20, 2019

efiring previously approved these changes Jan 20, 2019

View reviewed changes

jklymak mentioned this pull request Jan 20, 2019

MPL 3 + Colorbar + PowerNorm bug #13228

Closed

timhoffm reviewed Jan 20, 2019

View reviewed changes

jklymak force-pushed the fix-colorbar-norm-changed branch 2 times, most recently from 4356dcc to 712aceb Compare January 20, 2019 22:32

jklymak added the status: work in progress label Jan 20, 2019

NelleV previously approved these changes Jan 21, 2019

View reviewed changes

timhoffm reviewed Jan 21, 2019

View reviewed changes

timhoffm previously approved these changes Jan 21, 2019

View reviewed changes

jklymak removed the status: work in progress label Jan 21, 2019

NelleV previously approved these changes Jan 21, 2019

View reviewed changes

jklymak force-pushed the fix-colorbar-norm-changed branch from f6c3fd9 to 48b3510 Compare January 21, 2019 22:09

jklymak force-pushed the fix-colorbar-norm-changed branch from 00d62a7 to 0a8942d Compare January 23, 2019 23:00

efiring approved these changes Jan 26, 2019

View reviewed changes

jklymak force-pushed the fix-colorbar-norm-changed branch from 4f616b2 to 447747c Compare January 28, 2019 03:20

jklymak force-pushed the fix-colorbar-norm-changed branch from 447747c to d7e098c Compare January 31, 2019 01:34

LevN0 mentioned this pull request Feb 1, 2019

Log Colorbar minorticks_off reverted if ticks set... #13339

Closed

NelleV approved these changes Feb 2, 2019

View reviewed changes

lib/matplotlib/colorbar.py Outdated Show resolved Hide resolved

lib/matplotlib/colorbar.py Outdated Show resolved Hide resolved

DOC: slight docstring reformat

323b237

NelleV merged commit ce33de7 into matplotlib:master Feb 2, 2019

lumberbot-app bot added the status: needs manual backport label Feb 2, 2019

jklymak deleted the fix-colorbar-norm-changed branch February 2, 2019 21:19

jklymak pushed a commit to jklymak/matplotlib that referenced this pull request Feb 2, 2019

Merge pull request matplotlib#13234 from jklymak/fix-colorbar-norm-ch…

eb9bad4

…anged FIX: allow colorbar mappable norm to change and do right thing

jklymak modified the milestones: v3.0.3, v3.1 Feb 3, 2019

jklymak added topic: color/colorbar and removed status: needs manual backport labels Feb 3, 2019

ksunden mentioned this pull request Mar 29, 2019

Matplotlib deprecation for clim wright-group/WrightTools#890

Closed

ksunden mentioned this pull request Mar 29, 2019

BUG: Fix function signature mismatch for set_clim #13789

Merged

6 tasks

anntzer mentioned this pull request Apr 15, 2019

Dataless colorbars break when updated #13963

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: allow colorbar mappable norm to change and do right thing #13234

FIX: allow colorbar mappable norm to change and do right thing #13234

jklymak commented Jan 20, 2019 •

edited

Loading

jklymak commented Jan 20, 2019

efiring left a comment

timhoffm left a comment

NelleV commented Jan 20, 2019

jklymak commented Jan 20, 2019

timhoffm Jan 20, 2019

timhoffm commented Jan 20, 2019

NelleV left a comment

NelleV commented Jan 21, 2019

jklymak commented Jan 21, 2019

jklymak commented Jan 21, 2019

timhoffm Jan 21, 2019

timhoffm Jan 21, 2019

timhoffm Jan 21, 2019

timhoffm Jan 21, 2019

timhoffm Jan 21, 2019

timhoffm Jan 21, 2019

jklymak commented Jan 21, 2019

NelleV commented Jan 21, 2019

NelleV left a comment

jklymak commented Jan 24, 2019

jklymak commented Jan 25, 2019

efiring left a comment

efiring Jan 26, 2019

jklymak Jan 28, 2019

jklymak commented Jan 28, 2019

jklymak commented Jan 31, 2019 •

edited

Loading

LevN0 commented Feb 1, 2019

NelleV left a comment

NelleV commented Feb 2, 2019

lumberbot-app bot commented Feb 2, 2019

	the norm of the mappable will reeset the norm, locator, and formatters
	the norm of the mappable will reset the norm, locator, and formatters

	set the norm of the mappable associateed with this colorbar.
	Set the norm of the mappable associated with this colorbar.

	_log.debug('mappable changed')
	_log.debug('colorbar mappable changed')

	_log.debug('update normal')
	_log.debug('colorbar update normal')

	# note that that set_norm reemoves the FixedLocator...
	# note that set_norm removes the FixedLocator

	# make sure that format is passed properly....
	# make sure that format is passed properly

FIX: allow colorbar mappable norm to change and do right thing #13234

FIX: allow colorbar mappable norm to change and do right thing #13234

Conversation

jklymak commented Jan 20, 2019 • edited Loading

PR Summary

Before

After

Changing norm erases colorbar formatter and locator...

Next issue: Reset scale if norm changes...

Before

After

Next Issue: format='%1.4e' now works...

PR Checklist

jklymak commented Jan 20, 2019

efiring left a comment

Choose a reason for hiding this comment

timhoffm left a comment

Choose a reason for hiding this comment

NelleV commented Jan 20, 2019

jklymak commented Jan 20, 2019

Choose a reason for hiding this comment

timhoffm commented Jan 20, 2019

NelleV left a comment

Choose a reason for hiding this comment

NelleV commented Jan 21, 2019

jklymak commented Jan 21, 2019

jklymak commented Jan 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented Jan 21, 2019

NelleV commented Jan 21, 2019

NelleV left a comment

Choose a reason for hiding this comment

jklymak commented Jan 24, 2019

jklymak commented Jan 25, 2019

efiring left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented Jan 28, 2019

jklymak commented Jan 31, 2019 • edited Loading

LevN0 commented Feb 1, 2019

NelleV left a comment

Choose a reason for hiding this comment

NelleV commented Feb 2, 2019

lumberbot-app bot commented Feb 2, 2019

jklymak commented Jan 20, 2019 •

edited

Loading

jklymak commented Jan 31, 2019 •

edited

Loading