-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Warn in colorbar() when mappable.axes != figure.gca(). #12443
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
Conversation
c40c780
to
e416fab
Compare
lib/matplotlib/contour.py
Outdated
@@ -997,16 +997,21 @@ def __init__(self, ax, *args, | |||
warnings.warn('The following kwargs were not used by contour: ' + | |||
s) | |||
|
|||
@cbook.deprecated("3.1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we decide to deprecate self.ax
?
I'm -0.5 on this change. The implicit API is a bit goofy anyway, but there are plenty of reasons to have the colorbar steal from the current axes rather than the mappable's axes. I guess I think its more unexpected for plt.subplots(1, 2, 1)
pcm = plt.pcolormesh(X)
plt.subplots(1, 2, 2)
plt.plot(y)
plt.colorbar(pcm) to put the colorbar next to the first subplot than next to the one I specified last. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok for the warning.
-0.5 for on changing from self.ax
to self.axes
. This is quite inconsistently used throughout the code base (168 vs. 284 occurrences). If we touch this, I'd favor a separate PR going for all the changes. Changing only some here is just patchwork.
For the change to colorbar() to work we need ScalarMappables to have an While As for
if you make your plots vertically (211 and 212 instead of 121 and 122), I think the new behavior is better... TBH I don't really care about |
OK, I agree with that. I don't object to |
We haven't decided it, it's just needed for this specific PR to work (and again I can undeprecate If you insist we can also make plt.colorbar() use the current axes but fig.colorbar() the mappable's axes, but that seems worse... |
👍, but we should decide on the deprecation... |
Thanks for the explanation. Having |
e416fab
to
464072e
Compare
I'd be happy with the change but I'm sure someone will complain about useless code churn, etc :) |
fb5112d
to
40c0f45
Compare
40c0f45
to
9034a37
Compare
9034a37
to
52abed6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll set this on hold until we've decided if we want .ax
or .axes
. See #16058.
24b7edc
to
4a811b0
Compare
OK, I moved the relevant changes to #16058 and will rebase this one on top of it once it goes in. |
4a811b0
to
8a1034c
Compare
rebased |
8a1034c
to
96cae15
Compare
fe5b9e9
to
e3a5214
Compare
Actually, looking at this again, do we want to have the same behavior for pyplot.colorbar too? Obviously pyplot.colorbar gets to know what the current image (gci()) is, but it likely makes sense to always use gci()'s axes, not the current axes? (for the same reason why this makes sense for the OO API) |
Added the warning for pyplot.colorbar as a separate commit. |
738d7df
to
fa96f30
Compare
PR Summary
See discussion at #12333 (comment). Already 👍'd by @tacaswell :)
Edit: also closes #15986 indirectly.
Edit: will be rebased over #16058 once that goes in.
PR Checklist