[Bug]: colorbar with unattached mappables can't steal space #23709
Description
Bug summary
This is something I noticed downstream in networkx: networkx/networkx#5937 (comment)
Formerly, space for a Colorbar was stolen from the current Axes; that was deprecated and now in 3.6, it is stolen from the mappable's Axes. But if the mappable is not added to an Axes, it fails with a somewhat unrelated-looking error.
Code for reproduction
import matplotlib as mpl
import matplotlib.pyplot as plt
cmap = plt.get_cmap('viridis')
pc = mpl.collections.PatchCollection([], cmap=cmap)
pc.set_array([])
plt.colorbar(pc)Actual outcome
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/elliott/code/matplotlib/lib/matplotlib/pyplot.py", line 2053, in colorbar
ret = gcf().colorbar(mappable, cax=cax, ax=ax, **kwargs)
File "/home/elliott/code/matplotlib/lib/matplotlib/figure.py", line 1260, in colorbar
cax, kwargs = cbar.make_axes(ax, **kwargs)
File "/home/elliott/code/matplotlib/lib/matplotlib/colorbar.py", line 1396, in make_axes
fig = parents[0].get_figure()
AttributeError: 'NoneType' object has no attribute 'get_figure'Expected outcome
Either we should switch to the current Axes if the mappable doesn't have one (probably not desired given the previous change), or raise a clearer error message when this happens.
Additional information
No response
Operating system
No response
Matplotlib Version
3.6.0rc1
Matplotlib Backend
No response
Python version
No response
Jupyter version
No response
Installation
pip
Activity
oscargus commented on Aug 25, 2022
Not directly related to #23502, but it may be that the logic for stealing and returning space should be discussed jointly.
jklymak commented on Aug 25, 2022
Do we know when we changed to following the mappable's axes? That seems like we made a mistake there, and this may be a release critical bug fix. Marking as such to make sure it gets discussed.
anntzer commented on Aug 25, 2022
It was discussed at #12333 (comment) (see in particular @tacaswell's comment just below) and deprecated in #12443.
I still think the change is correct, we just need a better error message here. We could make plt.colorbar fallback to the current axes in case the current mappable has no axes if we really want to, but Figure.colorbar should not (as that's something that exists outside of the pyplot world).
jklymak commented on Aug 25, 2022
I think that issue refers to the problem of what axes to steal from whereas this one refers to which axes to give back to. We can't assume the logic is reversible because we have a cax arguement and an ax argument to colorbar.
anntzer commented on Aug 25, 2022
I'm not sure I follow? Isn't this issue still about who to steal from?
jklymak commented on Aug 25, 2022
I'm sorry - I got confused by the reference to the other issue
I think I'm ok with expecting the user to provide an axes if they just make an axes-less mappable and and expect plt.colorbar to do something. We could fall back to the current axes but in this case I think that would create an empty axes, which seems wrong as well. If they are creating their own mappable they have some savvy of our internals and can supply a cax argument.
tacaswell commented on Aug 25, 2022
I agree this seems wrong, but I think stealing from the current axes (as problematic as that is) even if we have to create it is better than failing. I could go either way on warning (and raising in the future) in the case of an "orphaned" mappable without an explicit axes passed or not.
anntzer commented on Aug 25, 2022
I'm OK with going through a warning before completely killing this
jklymak commented on Aug 25, 2022
I disagree. I think failing is better and the user can tell us what they want explicitly if we can't infer it.
tacaswell commented on Aug 25, 2022
Discussed on the call, I have been convinced we should give a better error as this was previously warned.
Clarify error for colorbar with unparented mappable
Clarify error for colorbar with unparented mappable
Clarify error for colorbar with unparented mappable
Clarify error for colorbar with unparented mappable
Clarify error for colorbar with unparented mappable
Clarify error for colorbar with unparented mappable