Skip to content

[Bug]: colorbar with unattached mappables can't steal space #23709

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

Jump to bottom
Closed
QuLogic opened this issue Aug 23, 2022 · 10 comments · Fixed by #23740
Closed

[Bug]: colorbar with unattached mappables can't steal space #23709

QuLogic opened this issue Aug 23, 2022 · 10 comments · Fixed by #23740
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: color/colorbar
Milestone
v3.6.0

Comments

@QuLogic
Copy link
Member

QuLogic commented Aug 23, 2022 •
edited
Loading

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
@QuLogic QuLogic added this to the v3.6.0 milestone Aug 23, 2022
@oscargus
Copy link
Member

Not directly related to #23502, but it may be that the logic for stealing and returning space should be discussed jointly.

@jklymak
Copy link
Member

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

@jklymak jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Aug 25, 2022
@anntzer
Copy link
Contributor

anntzer commented 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
Copy link
Member

jklymak commented 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
Copy link
Contributor

anntzer commented Aug 25, 2022

I'm not sure I follow? Isn't this issue still about who to steal from?

@jklymak
Copy link
Member

jklymak commented 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
Copy link
Member

but in this case I think that would create an empty axes, which seems wrong as well.

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
Copy link
Contributor

anntzer commented Aug 25, 2022

I'm OK with going through a warning before completely killing this

@jklymak
Copy link
Member

jklymak commented 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
Copy link
Member

Discussed on the call, I have been convinced we should give a better error as this was previously warned.

QuLogic added a commit to QuLogic/matplotlib that referenced this issue Aug 25, 2022
@QuLogic
Clarify error for colorbar with unparented mappable
e7bcb1e 
Fixes matplotlib#23709
QuLogic added a commit to QuLogic/matplotlib that referenced this issue Aug 25, 2022
@QuLogic
Clarify error for colorbar with unparented mappable
6ada95f 
Fixes matplotlib#23709
@QuLogic QuLogic mentioned this issue Aug 25, 2022
Merged
4 tasks
QuLogic added a commit to QuLogic/matplotlib that referenced this issue Aug 25, 2022
@QuLogic
Clarify error for colorbar with unparented mappable
5509ebb 
Fixes matplotlib#23709
@lucabaldini lucabaldini mentioned this issue Sep 23, 2022
Closed
melissawm pushed a commit to melissawm/matplotlib that referenced this issue Dec 19, 2022
@QuLogic @melissawm
Clarify error for colorbar with unparented mappable
4ba25ac 
Fixes matplotlib#23709
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: color/colorbar
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging a pull request may close this issue.

Clarify error for colorbar with unparented mappable QuLogic/matplotlib
5 participants
@tacaswell @QuLogic @anntzer @jklymak @oscargus