Skip to content

Add a colorbar() method to ScalarMappable? #25880

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.

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

Open
greglucas opened this issue May 13, 2023 · 20 comments
Open

Add a colorbar() method to ScalarMappable? #25880

greglucas opened this issue May 13, 2023 · 20 comments

Comments

@greglucas
Copy link
Contributor

greglucas commented May 13, 2023

Sorry to slightly hijack the thread here, but I think this may be a good opportunity to discuss whether
colorbar() could rather be a method on the ScalarMappable rather than on the Figure (we can always keep
figure.colorbar(...) available "forever", of course, for backcompat).
Indeed, in an call like fig.colorbar(mappable) or fig.colorbar(mappable, ax=...) or fig.colorbar(mappable, cax=...),
fig plays no role at all (*): the axes where the colorbar ends up is entirely determined by the mappable's axes (or by the ax
kwarg, or the cax kwarg.

(*) Actually I lied a bit: we currently check whether there's a layout engine on fig (but that actually seems more like a bug; we should really check that on mappable.axes.figure).

Originally posted by @anntzer in #25871 (comment)

@greglucas
Copy link
Contributor Author

greglucas commented May 13, 2023

I like this idea. I'd much rather do im.colorbar() than fig.colorbar(im). This actually is a pretty easy lift over to ScalarMappable. main...greglucas:matplotlib:scalarmappable-colorbar
However, the biggest hurdle is that ScalarMappable already has an attribute colorbar to store a reference to the last colorbar. So, is that worth deprecating/renaming and then implementing this after that transition?

@ksunden
Copy link
Member

ksunden commented May 13, 2023

Perhaps add_colorbar (or similar) allows clarity on the action being done by the method call while not touching the extant name colorbar

@ksunden
Copy link
Member

ksunden commented May 13, 2023

Not ideal to have a different name than fig.colorbar, of course, but perhaps better than the deprecation dance.

That said, ScalarMappable.colorbar doesn't seem to be used for for anything (only ever assigned to internally, didn't see any usage in examples or tests)

prior to v1.3 it was present but undocumented and returned a tuple of the colorbar and the colorbar axes, at that time it became an attribute and the set_colorbar method was deprecated (and subsequently removed in v1.5)

@jklymak
Copy link
Member

jklymak commented May 13, 2023

I don't particularly object to mappables gaining a colorbar method. However, I don't understand the idea that colorbars have nothing to do with the figure - they are axes that are added to the figure, just like all other axes and they need to interact with the other axes on a figure is complex ways. While we create colorbars using a mappable, it's important to keep in mind that they are often organizationally attached and can steal space from whole groups of axes.

@timhoffm
Copy link
Member

timhoffm commented May 13, 2023

I also think of the colorbar being conceptually attached to a figure (or axes).

As a user, I usually don't want to bother with the mappable explicitly (similar to plt.colorbar()). If there is only one mappable in the figure, it is obvios what the colormap should link to. I therefore propose a

While in pyplot we can do

plt.imshow(data)
plt.colorbar()

for the OO interface we currently have to

fig, ax = plt.subplots()
im = ax.imshow(data)
fig.colorbar(im)

I propose to go for

fig, ax = plt.subplots()
ax.imshow(data)
fig.colorbar()

with the semantic: "if no mappable is given, check whether there is exactly one mappable in the figure. If so, use that, if not, raise "Multiple mappables found in the figure, please specify the mappable explicitly which should be used for the colorbar". "

This follow the paradigm:

In many cases you will create a Figure and one or more Axes using pyplot.subplots and from then on only work on these objects.

I find this more intuitive than

fig, ax = plt.subplots()
im = ax.imshow(data)
im.colorbar()

@anntzer
Copy link
Contributor

anntzer commented May 13, 2023

However, I don't understand the idea that colorbars have nothing to do with the figure

I'm not saying that it has nothing to do with the figure, but rather that the figure to which it gets attached is entirely independent of the figure object on which colorbar() is called, and rather entirely determined by either (the figure owning) cax or ax (if set) or by the mappable('s figure) otherwise.

I usually don't want to bother with the mappable explicitly

Note that with the proposal here, you could conveniently use method chaining: ax.imshow(...).colorbar(...) (or plt.imshow(...).colorbar(...)).

if no mappable is given, check whether there is exactly one mappable in the figure. If so, use that, if not, raise

I think such an API would be poor, for the following reason: In isolation, you cannot know whether a call to fig.colorbar() is valid, as you need to know the entire history of fig (more specifically, how many mappables have been added to it). Sometimes, you can know that (if fig was created just above), but not always.

Similarly (it's really a variant on the same point), such an API would be quite brittle. Imagine that you write

fig, axs = plt.subplot_mosaic([["image", "plot"]])
<some more code>
axs["image"].imshow(...)
fig.colorbar()
<some more code>
axs["plot"].plot(...)

So far everything is good, but then you decide to add a third panel on the left with another image, which doesn't need a colorbar:

fig, axs = plt.subplot_mosaic([["otherimage", "image", "plot"]])
<some more code>
axs["otherimage"].imshow(...)
<some more code>
axs["image"].imshow(...)
fig.colorbar()
<some more code>
axs["plot"].plot(...)

Oops, all of a sudden the call to fig.colorbar() becomes invalid because of the ambiguity, even though it occurs far away from the changes that were made.

Still on the same idea, you can note that if "otherimage" was added on the right of "image", then the call to colorbar() would stay valid because at the moment of the colorbar() call there is still only one image in the figure, i.e. whether the call is valid or not also depends on the order of the subplots.

@story645
Copy link
Member

story645 commented May 14, 2023

However, I don't understand the idea that colorbars have nothing to do with the figure - they are axes that are added to the figure, just like all other axes and they need to interact with the other axes on a figure is complex ways.

I think that's sorta conflating two things though - semantically the colorbar is the legend for the colors in im (ETA: I'm thinking of this as $colorbar \approx cmap\circ norm$), technically it is implemented as a new axes to the figure. Or at least I'm reading your comment this way because I really like this proposal because I think im.colorbar puts that semantic association a little bit more forward.

Besides the technical concerns of a fig.colorbar that only works when there's one mappable, I think if this worked

fig, ax = plt.subplots()
im = ax.imshow(np.arange(0,100).reshape(10,10), cmap='RdBu')
fig.colorbar()

but this didn't:

fig, ax = plt.subplots()
im = ax.imshow(np.arange(0,100).reshape(10,10), cmap='RdBu')
sc = ax.scatter(np.linspace(0,9,5),np.linspace(9,0,5), c=np.linspace(0,1,5))
fig.colorbar()

we'd end up almost immediately w/ feature requests for the latter, regardless of the warning message. (ETA: I'd put one in)

@timhoffm
Copy link
Member

timhoffm commented May 14, 2023

Ok let's table parameter-less fig.colorbar() for now and discuss mappble.colorbar().

I'm strongly against mappble.colorbar().

It breaks with our dominant pattern that you add artists by calling methods on the parent artist that it gets added to (mostly Figure and Axes). From the artist hierarchy point of view, we don't add a colorbar to a mappable. Rather we add a colorbar to the figure and link it's content to a mappable.

To me mappable.colorbar() would be similar to a hypothetical plot(x, y).legend() (yes it's not directly comparable because legend can reference multiple artists but I hope you still get my point). This is not how our current logic works.

Additionally, for better or worse figure.colorbar(...) is an API we have to support indefinetely for backward-compatibility. mappable.colorbar() would introduce a second conceptual different approach, which is an extra -1 to me (there should only be one way). So even disregarding that it does not fit our logic, mappable.color() may be a more convenient shortcut, but it makes our API more cluttered and I believe this additional structure is a net minus on usability.

@anntzer
Copy link
Contributor

anntzer commented May 14, 2023

you add artists by calling methods on the parent artist that it gets added to

To be clear this is currently not the case for colorbars: as demonstrated by #25871 currently you can call a method on any figure (whether the mappable's (transitive) parent, or some unrelated figure), and the colorbar will get added to the mappable's parent figure, not to the one on which the method was called. We could add a check and error out if the
parent figure is different, as suggested at #25871 (comment), which I guess should be uncontroversial.

[On a more personal PoV, I find that just annoying for the end user: often I write

axs = plt.figure(...).subplots(...)  # I find it cleaner to split out kwargs for figure and kwargs for subplots
# or
ax = plt.figure(...).add_subplot(...)

then just work on ax/axs (I don't really need to carry around a fig variable which I mostly don't need), then if I need to write a colorbar I tend to just write ax.figure.colorbar(...) but really what comes before the colorbar call (i.e. ax.figure) is just line noise...]

I agree that the comparison with legend is a good one, but I guess this really means that the "clean" API would have been something like

cax = make_colorbar_axes(artist_or_axes_or_axeses)
# ... where make_colorbar_axes has to be a free function to handle the multi-axes case,
# or perhaps a figure method, with the same problems as now?
cax.colorbar(mappable)

as the colorbar really gets added to cax?

@tacaswell
Copy link
Member

The history here is that previously, fig.colorbar would default to stealing space from the figure's current Axes and in 3.4 we deprecated that (ttps://github.com//pull/12443) and in 3.6 switched to stealing space from the mappable's axes (#22081) so the coupling to the Figure used to be much tighter in the default case. We have smoothly deformed our selves to a slightly weird place API wise.

The funny thing here is that adding a colorbar under the correct situations adjust the Axes layout of a Figure (by resizing one or more existing Axes and adding a new Axes) so it is (in my view) correctly attached to a Figure (just like all of the other axes creating methods).

A comprimise that I suspect no one will like is to add Mappable.colorbar but make it require a cax and leave all of the auto-creation in Figure.colorbar.


Each axes does have a (private) notion of "current image" which is what plt.colorbar uses and Figures to have a notion of "current axes" so the example with multiple mappables in one axes or multiple axes with mappables is something that we have precedent for how to break those ties (rather than raising). That may not be something we want to do, but it is something we have done in the past. That said, I agree with @timhoffm we should table the "arguementless" discussion.

@jklymak
Copy link
Member

jklymak commented May 14, 2023

Isn't it simplest for mappable.colorbar to just be a short form for mappable.axes.figure.colorbar (probably not the correct attributes, but you get the idea)? It could then take all the arguments colorbar takes and default the ax argument to mappable.axes. That's a one liner and keeps the main method in figure.

@timhoffm
Copy link
Member

timhoffm commented May 14, 2023

you add artists by calling methods on the parent artist that it gets added to

To be clear this is currently not the case for colorbars [ ...] We could add a check and error out if the
parent figure is different, [...] which I guess should be uncontroversial.

Yes, we should add that check. I consider the current behavior a bug.


then just work on ax/axs (I don't really need to carry around a fig variable which I mostly don't need), then if I need to write a colorbar I tend to just write ax.figure.colorbar(...)

I can relate to this. Directs access to the figure is rarely needed, and the extra fig variable is often annoying (but for me fig, ax = plt.subplots(...) is still faster to type than ax = plt.figure(...).subplots(...) even if I don't use fig). It's a bit of a philosophical question whether one would always want fig for conceptual clarity/generality or whether one only wants ax and obtains the figure via ax.figure when needed. Anyway, plt.subplots() is likely the most common command and it returns a figure, so we're quite strongly tied into that.


I guess this really means that the "clean" API would have been something like [...]

Possibly, but that's even more unwieldly than fig.colorbar(mappable).

At the risk of dilluting the conversation:

The idea behind my parameter-less fig.colorbar() proposal was to

  1. build on top of work with the existing function - use what people already know instead of introducing another approach and
  2. make it simpler by not requiring mappable when it's unambiguous.

Yes, I fully agree that the drawbacks you mention in #25880 (comment) are there

In isolation, you cannot know whether a call to fig.colorbar() is valid, as you need to know the entire history of fig (more specifically, how many mappables have been added to it). Sometimes, you can know that (if fig was created just above), but not always.

I assume that is not too much of a problem in practice. (i) plt.colorbar() has the same issue that the current image could get changed and I'm not aware that there are complaints on that. (ii) It's your decision: if you don't oversee the context of the figure you should still provide mappable explicitly (we should document that). But I assume that most of the time, the figure code is just one short block and you have full oversight and control.

I agree that the comparison with legend is a good one,

We have fig.legend(). fig.colorbar() (without parameters) would be a nice analogy. - Yes, it does not work for more complicated cases with multiple mappables. But we can error and explain precisely why and give advice what the user has to do instead in the exception message. I believe users would be able to cope with that.

Weighting all the above against always having to specify mappable, the parameter-less fig.colorbar() still seems like a net win to me. - And, given the context of the existing library, is IMHO better than any other approach to colorbar that I've seen so far or could come up with.


Further remark (Now going to really dangerous territory - please don't jump on this if you don't like it - this is only a quick idea and not at all thought through)

I could even see an argument for a parameter-less ax.colorbar() method, taking the mappable from the axes. Advantages:

  • You would need the fig reference even less.
  • This search of the mappable is more narrowly scoped. - Less magic, less error-prone, supports multiple subplots with each having an image and a colorbar.
  • Analogy: We also have ax.legend()

Disadvantages:

  • An additional method
  • I'm risking throwing half of my "created through a method of the parent artist" argument under the bus. On a technical level, that argument is broken because colorbars are indepenent Axes and have the figure as parent. However, I'd argue that logically they are connected to the Axes in which the mappable lives that they are describing.

@anntzer
Copy link
Contributor

anntzer commented May 14, 2023

Each axes does have a (private) notion of "current image" which is what plt.colorbar uses and Figures to have a notion of "current axes" so the example with multiple mappables in one axes or multiple axes with mappables is something that we have precedent for how to break those ties (rather than raising). That may not be something we want to do, but it is something we have done in the past.

Hopefully it should be uncontroversial(!?!) that while the notions of current image/current axes/current figure is useful in pyplot and the pyplot API has its uses, these concepts should be kept as separated as possible from the OO core.

Isn't it simplest for mappable.colorbar to just be a short form for mappable.axes.figure.colorbar (probably not the correct attributes, but you get the idea)?

It can be that way or the other way round (figure.colorbar being a shorthand for mappable.colorbar, moving the implementation to ScalarMappable, note that you don't even need to get any attributes right here!), which is what I was thinking about, but obviously both ways are fine.

[Re: "clean" API] Possibly, but that's even more unwieldly than fig.colorbar(mappable).

Certainly. That was a bit of a strawman. (But I guess @tacaswell's "proposal that no-one will like" is basically along the same lines.)

plt.colorbar() has the same issue that the current image could get changed and I'm not aware that there are complaints on that.

That's literally the same problem we always have with the stateful pyplot API here. Certainly "in practice" calling plt.xlabel("foo") works for 98% of the people who are using the pyplot API and know what axes they're working on (because usually there's only a single axes anyways), but we still don't encourage that!
This goes back to my first point: I don't mind guessing in the pyplot API (I see "current axes/current image" basically as a formalized, guaranteed guess: we guess that the user wants to work on the axes/image they just worked on), but we should not build these concepts into the OO API. (I do realize that there's no guessing in the proposed argumentless fig.colorbar(), but anyways, hopefully that discussion is set aside for now ;))

an argument for a parameter-less ax.colorbar() method, taking the mappable from the axes

If anything I'd rather have that, because it is much rarer to have two mappables on the same axes (two mappables on the same figure is extremely common for me, I'd say). But I still don't like it :)

@timhoffm
Copy link
Member

Hopefully it should be uncontroversial(!?!) that [current*] concepts should be kept as separated as possible from the OO core.

Yes. That's uncontroversial. - This was only a close and simple example that inserting code may affect later commands. We have more than enough other cases where that happens.

@story645
Copy link
Member

story645 commented May 15, 2023

We have fig.legend(). fig.colorbar() (without parameters) would be a nice analogy. - Yes, it does not work for more complicated cases with multiple mappables.

See that's what I'm unhappy with since I think fig.legend grabs everything - and I'm maybe missing why fig.colorbar can't also grab everything and plot multiple colorbars?

ETA: I keep thinking ax.colorbar exists since fig.legend and ax.legend exist so I think that could be useful separate from this discussion.

@anntzer
Copy link
Contributor

anntzer commented May 15, 2023

why fig.colorbar can't also grab everything and plot multiple colorbars?

I think that's an interesting idea, but what would that call return? Returning a single colorbar in the case of a single mappable and a tuple/list of colorbars in the case of multiple mappables would result in a rather inelegant typing instability; always retuning a sequence of mappables would also not be so nice (cf. plot() which always returns a tuple of Line2Ds).

@timhoffm
Copy link
Member

why fig.colorbar can't also grab everything and plot multiple colorbars?

That does only work when there is only one mappable per Axes (which is ok - we can and should error out if things get ambiguous). In that case, it would be equivalent to a looped call on a hypothetical parameter-less ax.colorbar():

for ax in axs.flat:
    ax.colorbar()

At the risk of throwing more unfinshed API ideas around: For such a case, I have a rough vision, that we could replace the axs numpy array with a custom container. This container could grow vectorized functionality to save looping and calling the same function on all elements, like we already have for the Spines container.

This would then allow

axs[:,:].colorbar()

and of course variants like only setting colorbars at the most-right plots (axs[:,-1].colorbar()).

@anntzer
Copy link
Contributor

anntzer commented May 15, 2023

That does only work when there is only one mappable per Axes

My interpretation of the proposal was rather to grab all mappables in the Axes and add a colorbar for each (and just have the colorbars next to one another).

@timhoffm
Copy link
Member

timhoffm commented May 15, 2023

That does only work when there is only one mappable per Axes

My interpretation of the proposal was rather to grab all mappables in the Axes and add a colorbar for each (and just have the colorbars next to one another).

I see. That would be an extension/wider API of the parameter-less approach. My thought was keeping it simple and just not support multiple mappables in one Axes through one high-level function call. One can expand that, but that adds complexity (e.g. you cannot use cax for multiple colorbars; what is the order when adding multiple colorbars next to one Axes? does that have to be configurable?). Overall, I wonder how important the "multiple mappables with colorbars in one Axes" case is practice. Do you have real-world use cases? If we want this, I'd still do this in two steps/PRs: First support one mappable per Axes; when that works think about all the corner cases we have to care about for multiple mappables.

@story645
Copy link
Member

story645 commented May 15, 2023

what is the order when adding multiple colorbars next to one Axes? does that have to be configurable?

I was thinking it could have a similar but simplified version of the legend API, so for example given these two mabbles:

im = ax.imshow(np.arange(0,100).reshape(10,10), cmap='RdBu')
sc = ax.scatter(np.linspace(0,9,5),np.linspace(9,0,5), c=np.linspace(0,1,5))

these should be roughly equivalent calls and yes I'm also proposing that you can pass in a list of colorbars cause I think having to call the same function twice when you don't have to for legend seems weird unless you know the MPL internals.

current proposed with ordering
fig.colorbar(im); fig.colorbar(sc) fig.colorbar() fig.colorbar([im, sc])

Do you have real-world use cases?

map showing two meteorology variables, complex heatmap r package -> though both of these examples arrange colorbars more like legend entries
proplot has a good colorbar legend comparison

you cannot use cax for multiple colorbars
First support one mappable per Axes; when that works think about all the corner cases we have to care about for multiple mappables.

Yeah, I didn't know this and to Anthony's point about type instability and folks who do multiple colorbars wanting a legend type situation, if the answer for multiple colorbars isn't to have a legend like container object, but I also don't think that would work for the one horizontal/one vertical case either. I'm also not opposed to making this a new function (fig.colorbar_legend) instead.

Going back to the original proposal of im.colorbar,

However, I'd argue that logically they are connected to the Axes in which the mappable lives that they are describing

I like sm.colorbar() b/c I think that's the clearest way to denote that the mappable is the controlling artist for a colorbar, even if the colorbar is a child of a different Artist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants