Fix cla colorbar

[jklymak]

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

[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]

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

[jklymak]

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]

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]

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]

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]

Sure, that works fine on this PR.

[greglucas]

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?

[jklymak]

Oh more lovely callbacks ;-)

[jklymak]

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.

[jklymak]

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)

[jklymak]

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

[greglucas]

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

[jklymak]

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.

[greglucas]

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

[jklymak]

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.

[greglucas]

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]

Pretty sure this is obsoleted by #20501