FIX: allow colorbar mappable norm to change and do right thing by jklymak · Pull Request #13234 · matplotlib/matplotlib · GitHub

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

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

Looks good.

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

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)

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

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

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.

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

@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?)

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!

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...

this comment is not true anymore, I think.

Suggested change

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

Suggested change

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

Giving slightly more context helps understanding the log output.

Suggested change

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

Giving slightly more context helps understanding the log output.

Suggested change

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

Suggested change

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

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.

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

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

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.

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

review is too old.

stale

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

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

Fixed both points! Thanks,

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

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

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

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

Thanks!

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

Comment thread lib/matplotlib/colorbar.py Outdated

Comment thread lib/matplotlib/colorbar.py Outdated

DOC: slight docstring reformat

323b237

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

lumberbot-app Bot added the Still 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 Still 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

                       self.outline = None
                       self.patch = None
                       self.dividers = None
+                      # self.locator and self.formatter are properties...

+                  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...

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

	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



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

FIX: allow colorbar mappable norm to change and do right thing by jklymak · Pull Request #13234 · matplotlib/matplotlib · GitHub

Repository navigation

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

FIX: allow colorbar mappable norm to change and do right thing#13234NelleV merged 2 commits intomatplotlib:mastermatplotlib/matplotlib:masterfrom jklymak:fix-colorbar-norm-changedjklymak/matplotlib:fix-colorbar-norm-changedCopy head branch name to clipboard

Conversation

jklymak commented Jan 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

jklymak commented Jan 20, 2019

Uh oh!

efiring left a comment

Choose a reason for hiding this comment

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

NelleV commented Jan 20, 2019

Uh oh!

jklymak commented Jan 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timhoffm commented Jan 20, 2019

Uh oh!

NelleV left a comment

Choose a reason for hiding this comment

Uh oh!

NelleV commented Jan 21, 2019

Uh oh!

jklymak commented Jan 21, 2019

Uh oh!

jklymak commented Jan 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak commented Jan 21, 2019

Uh oh!

NelleV commented Jan 21, 2019

Uh oh!

NelleV left a comment

Choose a reason for hiding this comment

Uh oh!

jklymak commented Jan 24, 2019

Uh oh!

jklymak commented Jan 25, 2019

Uh oh!

efiring left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak commented Jan 28, 2019

Uh oh!

jklymak commented Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LevN0 commented Feb 1, 2019

Uh oh!

NelleV left a comment

FIX: allow colorbar mappable norm to change and do right thing#13234
NelleV merged 2 commits into
matplotlib:masterfrom
jklymak:fix-colorbar-norm-changed

jklymak commented Jan 20, 2019 •
edited
Loading

jklymak commented Jan 31, 2019 •
edited
Loading