Skip to content

Fix cla colorbar #20330

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.

Sign up for GitHub

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

Closed
jklymak wants to merge 6 commits into from
Closed

Fix cla colorbar #20330

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c750a34
FIX: add cla to colorbar
jklymak May 28, 2021
549e0e7
TST: add test
jklymak May 28, 2021
a0b05e4
fix
jklymak May 28, 2021
1675c3a
??
jklymak Jun 1, 2021
f0b91dc
FIX: make scalarmappable callbacks know all colorbars
jklymak Jun 1, 2021
8ae6b42
FL8
jklymak Jun 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
FIX: add cla to colorbar 
co-author: Greg Lucas <greg.m.lucas@gmail.com>
  • Loading branch information
@jklymak
jklymak committed May 28, 2021
commit c750a34df6f17d25967e0874e6ac19887571c54e
34 changes: 32 additions & 2 deletions lib/matplotlib/colorbar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,6 @@ def __init__(self, parent, userax=True):
parent.remove()
else:
outer_ax = parent

inner_ax = outer_ax.inset_axes([0, 0, 1, 1])
self.__dict__.update(inner_ax.__dict__)

Expand All @@ -285,6 +284,32 @@ def _set_inner_bounds(self, bounds):
self.inner_ax._axes_locator = _TransformedBoundsLocator(
bounds, self.outer_ax.transAxes)

def cla(self):
"""
Reset the colorbar axes to be empty.
"""
# need to low-level manipulate the stacks because we
# are just swapping places here. We don't need to
# set transforms etc...
self.inner_ax.cla()
self.outer_ax.cla()
self.figure._axstack.add(self)
self.figure._axstack.remove(self.outer_ax)
self.figure._localaxes.add(self)
self.figure._localaxes.remove(self.outer_ax)

self.__dict__.update(self.outer_ax.__dict__)
self.inner_ax.remove()
del self.inner_ax
del self.outer_ax

self.xaxis.set_visible(True)

@greglucas greglucas May 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self.yaxis.set_visible(True)
for spine in self.spines.values():
spine.set_visible(True)

self.set_facecolor(mpl.rcParams['axes.facecolor'])


class _ColorbarSpine(mspines.Spine):
def __init__(self, axes):
Expand Down Expand Up @@ -1250,7 +1275,12 @@ def update_normal(self, mappable):
self.norm = mappable.norm
self._reset_locator_formatter_scale()

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

@greglucas greglucas May 31, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jklymak May 31, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh more lovely callbacks ;-)

@jklymak jklymak May 31, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jklymak Jun 1, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jklymak Jun 1, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@greglucas greglucas Jun 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jklymak Jun 1, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 greglucas Jun 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jklymak Jun 1, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 greglucas Jun 2, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

pass

if isinstance(self.mappable, contour.ContourSet):
CS = self.mappable
if not CS.filled:
Expand Down