Conversation
anntzer
left a comment
There was a problem hiding this comment.
As discussed quite extensively at #25880 (in particular #25880 (comment), which is about parameterless Figure.colorbar(), though the same arguments also apply here) I really don't like this idea. I don't think it's worth starting a new thread here to rehash the arguments, so perhaps that can be sorted out in the original thread?
aa24c65 to
1506eb5
Compare
|
Thanks for bringing that discussion to attention. I agree today that a parameterless Since #25880 discusses various related topics and #25880 (comment) is burried deep in, I think it's cleaner to sort out the specific question here: Why does a parameterless Argument: Removing the mappable breaks the explicit connection to the Figure/Axes that the colorbar is placed in.
Reply: This is true for We could stop here. But similar to Leaving out the mappable does not break the bond, because the strong and unique bond of Note that both Argument: Brittle API:
Reply: This is a valid concern on figures. It is much less severe on a single Axes for two reaons:
Note that the parameterless I hope I have correctly extracted the major arguments against parameterless
|
c17bf6a to
4f65ab4
Compare
66e50cf to
e44ca23
Compare
e44ca23 to
3beaebf
Compare
jklymak
left a comment
There was a problem hiding this comment.
I'd approve this - I think this is a reasonable shortcut, doesn't preclude adding a mappable.colorbar (though I'm sort of against that), and I think erroring in the case of ambiguity is a good idea. I'd suggest that cax be kept and override like it does for the figure.colorbar.
This is another way to do something - I'm a little concerned that if we replace every example with this new shortcut is a bit too aggressive - every example on the internet etc has the old grammar so it makes us look inconsistent. I also am mildly concerned that people will miss the power of the figure.colorbar method, which can position colorbars at the expense of multiple axes, which is really quite powerful.
| mappable = colormapped_artists[0] | ||
|
|
||
| return self.figure.colorbar( | ||
| mappable, ax=self, use_gridspec=use_gridspec, **kwargs |
There was a problem hiding this comment.
I think we should allow the cax keyword argument like for fig.colorbar for manual placement.
I think changing every one axes example to use this method could highlight the power of fig.colorbar b/c the examples are then mostly limited to showing off this functionality. It allows for a clearer separation of concerns. I'm less worried about the multiple ways to do things mostly b/c I think this method is so intuitive (akin to the axes.legend) that I tend to forget it doesn't exist. |
Certainly, we all agree that
There's a fundamental difference with
These points are hard to argue about, because they are based on generalities ("usually", "typically"). I will however admit your feeling is likely correct here.
I don't think these statistics mean much (despite my point just above); e.g. I can find ~4x more uses of
But There's another issue specifically regarding |
Just as a side note but this proposal should probably be colorizer.colorbar() b/c the colorizer interface is what holds the color mapping now |
|
I answer the two key points in detail and reply to the other arguements briefly after that. Why ax.colorbar() should not support caxI have intentionally not added a cax parameter, because that would muddy the water. It prevents the "there's actually two possible axes" point laid out by @anntzer. Additionally, there's a hierarchy of generality in colorbar placement
(2.) on an axes method would be really weird ( Comparing
|
|
Re:
From that perspective, allowing |
|
So far, I have thought of ax.colorbar as the (common) special case of having the colorbar next to a single Axes. fig.colorbar is then the generalization of placing a colorbar anywhere in the figure. |
|
No, I wouldn't support that because it'd be confusing to have that colorbar "belong" to For sure If we switch this method, I'd do It's definitely not the end of the world to do the extra steps (making |
Then, wouldn't it help more to have an easier way for creating inset colorbars built into ax.colorbar? Just spitballing, but we could e.g. support a coordinate tuple in axes coordinates in the position argument, so that you could do But generally, starting with a narrow API footprint leaves us the freedom to exapand in a suitable direction later. Whether that is cax, or position or something else or keeping it narrow. |
In the same way, plt.plot() is highly used; we know that many people find the explicit axes concept awkward and prefer the implicit API, but we don't want to encourage it, because we find the implicit API awkward.
That looks even worse than I don't want to rehash the same arguments again and again. I do feel quite strongly about this, so I propose (instead of fully blocking the PR) to take my no as a -1 vote and require 3 positive reviews instead of 2 to merge this. (By the way, I notice that the review procedure at https://matplotlib.org/devdocs/devel/pr_guide.html#approval says nothing about PR rejections, other than in the "championed PR" case.) |
|
PR rejections: I believe we have so far tried to reach a consensus - in more difficult cases and to get broader opinions through discussion in the weekly meeting. Happy to do that to get broader attention and feedback to the topic. If no consensus can be reached, I think the eventual decision lies with the respective deputy project leader per:
(Disclosure: I believe this is in the responsibility of the API leader, which happens to be me) |
|
@anntzer do you object to a method on the axes or just the fact it is implicitly picking up the mappable in this implementation? I personally don't have any objection to a shortcut to the main method. My preference would be for it to be as close to the figure method as possible. There is ambiguous and unambiguous implicit API. If this implicitly chose the last mappable I would complain because that is arbitrary and can lead to confusion if the user changes the order of calls in their code. It is the same concern I have with the plt interface - the user needs to track what happened before to guess what the method will do. Here, the implicitness is unambiguous, or errors if ambiguous. So my only concern about it being implicit is that it doesn't teach users how to do more complex colorbar mappings. With respect to mappable.colorbar that seems orthogonal in that you could have both ax.colorbar and mappable.colorbar. |
|
To be clear, I'm not going to be mad at anyone if this ends up going in with just two positive reviews. It's not the end of the world at all, I just want to register my strong opposition to it. I have objections both against the fact that it's an Axes method, and against the fact that it tries to pick up a single mappable. Both points have already been made above, but to summarize them here: (a) The fact that it's an axes method is awkward because of the Admittedly, the single mappable selection is likely to work well in practice, and thinking about it again it should be much less problematic than an argument-less |
I agree that aesthetically it is a bit ugly, but I'd argue it's relatively rare, and doesn't hurt anything.
I guess I'm not 100% sure what you mean here. I guess for interactive use it become brittle in that if someone does it doesn't error out and the colorbar is for they get an error? I guess that is a bit ambiguous but I'm not convinced it means we should throw out the idea of an implicit mappable. |
|
Generally, order of matplotlib commands does matter:
Such state-dependent behavior is deeply baked in. One would need a completely different architecture to prevent this. |
|
My point regarding brittleness is the following: In fact, the case of scatter() makes me realize another issue with parameter-less ax.scatter(x0, y0)
ax.scatter(x1, y1, c=...)
ax.colorbar()will break, even though the user intent is clearly that there's only one mappable here (the one with |
|
I think there are lots of cases where a user and a helper function can conflict with each other. If I were using a helper that asked for an axes, I would not make assumptions about what would survive the helper. Conversely, if I were writing a helper, I'd try and avoid implicit assumptions if I could. In this case, I still feel the helper function would error in a sensible way, and either the user or the author of the helper could each fix from their ends. (The use of scatter with and without a colormapping is definitely an edge case, but again, is pretty easy to fix). |
|
unsafe helper functions The helper function is still easily fixable and can be made safe by passing the mappable
So even if that was missed initially, it's straight forward to recover from. I believe, the implicit mappable detection is valuable enough for simple cases so that we shouldn't exclude it just because someone can - but doesn't have to - use is insecurely in more complex scenarios. issues with non-mapping images and collections I had already thought about the scatter case. Both, images and Collections can use colormapping, but don't have to. It's "opt-in" for collections by providing data ( The correct approach is to detect "is there exactly one artist that uses colormapping". This is in principle possible, we just don't have a nice API to query an Artist whether it uses colormapping. I didn't want to load the PR and discussion with that extra compexity. This is rather an edge case, and the worst that can happen is that the implicit detection complains and you have to explicity pass the mappable, which is not making things worse compared to now. We can later refine the detection, and that's actually my plan. |
Up to now, we have
pyplot.colorbar()andFigure.colorbar().https://matplotlib.org/stable/api/_as_gen/matplotlib.pyplot.colorbar.html is convenient in that one can do
and it automatically uses the current image.
With https://matplotlib.org/stable/api/_as_gen/matplotlib.figure.Figure.colorbar.html one has to explicitly pass the image, leading to somewhat bulky
This PR replicates the ease of use of
plt.colorbar()in the OOP interface on an Axes: While we don't have the "current image" notion. Intent is still unambiguos if there is exactly one mappable in the Axes, which is the most common case. So one can doNote also that
ax.colorbar()is somewhat analogous toax.legend(): Add an explaining context Artist, based on the information that is already in the Axes; i.e. all parameters are optional.Reviewing: This PR consists of to commits:
Use the examples to get a feeling how this will work in practice.