Fix cla colorbar #20330

jklymak · 2021-05-28T23:31:18Z

PR Summary

Alternative to #20323.

This allows calling cb.ax.cla and reusing the same axes to swap in a new colorbar. If the colorbar is not filled in, an empty axes with the default facecolor is left where the old one was.

Co-authored with @greglucas since I stole his test (though he is welcome to disavow the PR ;-))

PR Checklist

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (run flake8 on changed files to check).
New features are documented, with examples if plot related.
Documentation is sphinx and numpydoc compliant (the docs should build without error).
Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

co-author: Greg Lucas <greg.m.lucas@gmail.com>

greglucas

I think this is a better fix 👍. Could you also add the remove() method on the axes too? I think that is the one I was running into actually.

greglucas · 2021-05-28T23:54:37Z

lib/matplotlib/colorbar.py

+        del self.inner_ax
+        del self.outer_ax
+
+        self.xaxis.set_visible(True)


Here self is back to an Axes now, so can you call super().cla() to get the defaults for you?

jklymak · 2021-05-29T00:11:27Z

We can probably make that work. I think it's a little trickier than what you had in the other PR. What is the goal of removing the axes from the axes versus calling cb.remove()?

jklymak · 2021-05-29T02:02:55Z

Actually, I'm a little confused what you want cb.ax.remove to do. cb.remove removes the colorbar , and resizes the mappables axes to refill the space. So far, what I have for cb.ax.remove it just removes the colorbar, but it doesn't resize the mappables axes. Is that what you want?

greglucas · 2021-05-29T02:26:28Z

The issue I was running into was that parent.remove() was failing when I was trying to replicate the axes_grid test with a normal colorbar axis.

matplotlib/lib/matplotlib/colorbar.py

Line 260 in 08f4629

parent.remove()

I assumed that was because the remove() method wasn't on CbarAxes, but now that I think about it I'm not sure why that method wouldn't be inherited from the base Axes in the first place for something to be there already. It may be that the cla() method here is enough to clean up that use case, which I think is shown by your test not failing...

greglucas · 2021-05-29T02:29:55Z

Here is my reproducing example on master currently.

import matplotlib.pyplot as plt
import numpy as np

X, Y = np.meshgrid(np.linspace(0, 6, 30), np.linspace(0, 6, 30))
arr = np.sin(X) * np.cos(Y) + 1j*(np.sin(3*Y) * np.cos(Y/2.))

fig, axarr = plt.subplots(figsize=(12, 9), nrows=2, ncols=2)


im1 = axarr[0, 0].imshow(arr.real, cmap='nipy_spectral')
im2 = axarr[1, 0].imshow(arr.imag, cmap='hot')
im3 = axarr[0, 1].imshow(np.abs(arr), cmap='jet')
im4 = axarr[1, 1].imshow(np.arctan2(arr.imag, arr.real), cmap='hsv')

cb1 = plt.colorbar(im1, ax=axarr[:, 0])
cb2 = plt.colorbar(im2, ax=axarr[:, 1])

cb1.ax.cla()
cb3 = plt.colorbar(im3, cax=cb1.ax)
plt.show()

jklymak · 2021-05-29T03:11:18Z

Sure, that works fine on this PR.

greglucas · 2021-05-31T14:04:09Z

lib/matplotlib/colorbar.py

+        try:
+            self.draw_all()
+        except AttributeError:
+            # update_normal sometimes is called when it shouldn't be..


Why does update_normal get called when it shouldn't be? Presumably it has something to do with the new cla() addition you added. I wonder if you need to remove the callbacks_cid from callbacksSM?

Oh more lovely callbacks ;-)

The fundamental problem is that the colorbar axes is cla, but the colorbar object is not removed. Then Colorbar__init__ calls mappable.autoscale_None() which triggers a draw of the orphaned colorbar. The orphaned colorbar tries to draw itself, but of course all its axes have been destroyed and we get an AttributeError.

The problem is that for the callbacks, each mappable only maps to one colorbar, but in the following the mappable is used for two colorbars:

import matplotlib.pyplot as plt import numpy as np fig, ax = plt.subplots() pc = ax.imshow(np.arange(100).reshape(10, 10)) cb = fig.colorbar(pc, extend='both') cb2 = fig.colorbar(pc) # Clear and re-use the same colorbar axes print('HERE1:', pc, pc.callbacksSM.callbacks) cb.ax.cla() print('HERE2:', pc, pc.callbacksSM.callbacks) cb2.ax.cla() print('HERE3:', pc, pc.callbacksSM.callbacks) cb = fig.colorbar(pc, cax=cb.ax) cb2 = fig.colorbar(pc, cax=cb2.ax)

which results in:

HERE1: AxesImage(80,52.8;317.44x369.6) {'changed': {0: <weakref at 0x1085d69e0; to 'Colorbar' at 0x1096758e0>, 1: <weakref at 0x109709120; to 'Colorbar' at 0x109748130>}} id 1 HERE2: AxesImage(80,52.8;317.44x369.6) {'changed': {0: <weakref at 0x1085d69e0; to 'Colorbar' at 0x1096758e0>}} id 1 HERE3: AxesImage(80,52.8;317.44x369.6) {'changed': {0: <weakref at 0x1085d69e0; to 'Colorbar' at 0x1096758e0>}}

For HERE3 there should be no callback, but it never gets dicsonnected when we do cb2.ax.cla() because pc doesn't know about cb2.

The culprit is here:

matplotlib/lib/matplotlib/colorbar.py

Lines 1181 to 1183 in d0137e9

mappable.colorbar = self

mappable.colorbar_cid = mappable.callbacksSM.connect(

'changed', self.update_normal)

Of course this doesn't work at all on master? So maybe this test is just too stringent.

Here is an idea that would help this situation (and the pan/zoom on colorbars too which would help me). What about making the Colorbar object itself inherit from CbarAxes/become an Axes object? Then you would only call methods on the Colorbar object, not cbar.ax/cax...

I don't know if that helps in particular.

The issue here is that the scalar mappable is only storing a reference to a single colorbar, whereas it stores two callbacks. So the line self.colorbar.mappable.callbacksSM.disconnect(self.colorbar.mappable.colorbar_cid) only knows about one colorbar id, and loses track of the other colorbar. That leads to the stray draw.

The newest commit promotes colorbar_cid to a dictionary if there is more than one and .colorbar to a list. Thats a bit of a breaking change for what is stored on this attribute, but makes sure we don't lose track of what colorbars are on the scalar mappable.

Oof, this opened a can of worms ;)

I don't think changing colorbar_cid to a dictionary is a great idea here, that seems like a lot more logic/work going on under the hood to take care of a niche case. As it so happens, it looks like there is some discussion about multiple callbacks in #20210 as well.

For this specific use-case, the reason you're getting multiple callbacks is because when you call cbar.ax.cla() that ax.cla() has no idea it should also clear a callback on the cbar. But, if we move that up a level to make it just cbar.cla() (i.e. Colorbar is now an Axes object), that clear call would know to get rid of the callback (and also clear the inner/outer axes as you already have implemented).

I'm not at all fundamentally against that, but it seems a pretty hefty change. I think what you would do is lock out cb.ax and then just reproduce all the axes methods (which has been done already, but only partially). But it'd take some work to do it and not have a bunch of breaking API.

Surprisingly, it doesn't seem that bad... See this commit:
greglucas@569fbc1
Passes all test_colorbar.py tests locally. Just pointing self.ax = self after initialization would help and we could probably stick in a deprecation warning pass-through on that too. Maybe this is getting a little side-tracked for this thread and I should open up a separate issue for discussion?

jklymak · 2021-07-28T20:00:55Z

Pretty sure this is obsoleted by #20501

jklymak added the topic: color/colorbar label May 28, 2021

jklymak added this to the v3.5.0 milestone May 28, 2021

jklymak added 2 commits May 28, 2021 16:33

FIX: add cla to colorbar

c750a34

co-author: Greg Lucas <greg.m.lucas@gmail.com>

TST: add test

549e0e7

jklymak force-pushed the fix-cla-colorbar branch from 827f571 to 549e0e7 Compare May 28, 2021 23:34

fix

a0b05e4

greglucas reviewed May 28, 2021

View reviewed changes

greglucas mentioned this pull request May 28, 2021

Adding cla and remove to ColorbarAxes #20323

Closed

4 tasks

greglucas reviewed May 31, 2021

View reviewed changes

jklymak added 3 commits May 31, 2021 18:24

??

1675c3a

FIX: make scalarmappable callbacks know all colorbars

f0b91dc

FL8

8ae6b42

This was referenced Jun 2, 2021

Have Colorbar inherit from Axes #20341

Open

Colorbar inherit from Axes #20350

Draft

jklymak marked this pull request as draft June 10, 2021 14:19

jklymak closed this Jul 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cla colorbar #20330

Fix cla colorbar #20330

jklymak commented May 28, 2021

greglucas left a comment

greglucas May 28, 2021

jklymak commented May 29, 2021

jklymak commented May 29, 2021

greglucas commented May 29, 2021

greglucas commented May 29, 2021

jklymak commented May 29, 2021

greglucas May 31, 2021

jklymak May 31, 2021

jklymak May 31, 2021

jklymak Jun 1, 2021

jklymak Jun 1, 2021

greglucas Jun 1, 2021

jklymak Jun 1, 2021

greglucas Jun 1, 2021

jklymak Jun 1, 2021

greglucas Jun 2, 2021

jklymak commented Jul 28, 2021

	mappable.colorbar = self
	mappable.colorbar_cid = mappable.callbacksSM.connect(
	'changed', self.update_normal)

Fix cla colorbar #20330

Fix cla colorbar #20330

Conversation

jklymak commented May 28, 2021

PR Summary

PR Checklist

greglucas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented May 29, 2021

jklymak commented May 29, 2021

greglucas commented May 29, 2021

greglucas commented May 29, 2021

jklymak commented May 29, 2021

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

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 Jul 28, 2021