Skip to content

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

Merged
merged 2 commits into from
Jul 18, 2020

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Oct 8, 2018

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

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer anntzer force-pushed the colorbar-mappable-axes branch from c40c780 to e416fab Compare October 8, 2018 12:50
@@ -997,16 +997,21 @@ def __init__(self, ax, *args,
warnings.warn('The following kwargs were not used by contour: ' +
s)

@cbook.deprecated("3.1")
Copy link
Member

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?

@jklymak
Copy link
Member

jklymak commented Oct 8, 2018

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.

Copy link
Member

@timhoffm timhoffm left a 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.

@anntzer
Copy link
Contributor Author

anntzer commented Oct 8, 2018

For the change to colorbar() to work we need ScalarMappables to have an .axes, so ContourSets need to grow such an attribute (... or we need to special-case ContourSets in the colorbar() code, which seems worse). We don't have to deprecate .ax, we can leave both of them around, but it seems just as good to make things consistent.
I can undeprecate ContourSet.ax if that's the main issue...

While .ax and .axes may be nearly as common in raw counts in the codebase, I'd guess .axes is much more common in actual use because that's what Artists have, and Artists also share a lot of code in the base class, whereas .ax is on a couple of custom non-Artist things (e.g. ContourSets) with each their own codepaths.

As for

plt.subplots(1, 2, 1)
pcm = plt.pcolormesh(X)
plt.subplots(1, 2, 2)
plt.plot(y)
plt.colorbar(pcm) 

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 plt.colorbar(), but at least for fig.colorbar() I'd rather not invoke the notion of "current axes".

@jklymak
Copy link
Member

jklymak commented Oct 8, 2018

TBH I don't really care about plt.colorbar(), but at least for fig.colorbar() I'd rather not invoke the notion of "current axes".

OK, I agree with that.

I don't object to axes being universal versus ax, but I wasn't aware we had decided that. And as noted above an effort should be made to do it everywhere. Understood about needing the attribute.

@anntzer
Copy link
Contributor Author

anntzer commented Oct 8, 2018

We haven't decided it, it's just needed for this specific PR to work (and again I can undeprecate .ax for this specific PR if you prefer). Making the change everywhere is absolutely not the point of this PR.

If you insist we can also make plt.colorbar() use the current axes but fig.colorbar() the mappable's axes, but that seems worse...

@jklymak
Copy link
Member

jklymak commented Oct 8, 2018

👍, but we should decide on the deprecation...

@timhoffm
Copy link
Member

timhoffm commented Oct 8, 2018

For the change to colorbar() to work we need ScalarMappables to have an .axes, so ContourSets need to grow such an attribute.

Thanks for the explanation. Having ax and axes is not good either. Even though it's not the target of this PR, can we decide on a global change from ax to axes in attribute names? That would allow to keep the deprecation in here and add the remaining ones in another PR.

@anntzer anntzer force-pushed the colorbar-mappable-axes branch from e416fab to 464072e Compare October 8, 2018 21:27
@anntzer
Copy link
Contributor Author

anntzer commented Oct 8, 2018

I'd be happy with the change but I'm sure someone will complain about useless code churn, etc :)

Copy link
Member

@timhoffm timhoffm left a 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.

@anntzer anntzer force-pushed the colorbar-mappable-axes branch 3 times, most recently from 24b7edc to 4a811b0 Compare January 13, 2020 06:05
@timhoffm
Copy link
Member

timhoffm commented Mar 25, 2020

@anntzer To move this forward, you could split out the ax -> axes deprecation and move that to #16058. It's better to discuss it in that context.

@anntzer
Copy link
Contributor Author

anntzer commented Mar 25, 2020

OK, I moved the relevant changes to #16058 and will rebase this one on top of it once it goes in.

@anntzer
Copy link
Contributor Author

anntzer commented Jun 25, 2020

rebased

@tacaswell tacaswell added this to the v3.4.0 milestone Jun 26, 2020
@tacaswell tacaswell force-pushed the colorbar-mappable-axes branch from 8a1034c to 96cae15 Compare June 26, 2020 01:33
@QuLogic QuLogic requested a review from timhoffm June 27, 2020 05:35
@anntzer anntzer force-pushed the colorbar-mappable-axes branch 2 times, most recently from fe5b9e9 to e3a5214 Compare July 2, 2020 20:38
@anntzer
Copy link
Contributor Author

anntzer commented Jul 2, 2020

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)

@anntzer
Copy link
Contributor Author

anntzer commented Jul 6, 2020

Added the warning for pyplot.colorbar as a separate commit.
Edit: This actually revealed a bug in the original implementation: we should not warn if cax is set, as in that case we're not going to steal from ax anyways. Fixed.

@anntzer anntzer force-pushed the colorbar-mappable-axes branch from 738d7df to fa96f30 Compare July 6, 2020 08:50
@QuLogic QuLogic merged commit 98961f6 into matplotlib:master Jul 18, 2020
@anntzer anntzer deleted the colorbar-mappable-axes branch July 18, 2020 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

old colorbar removed but still occupies space
5 participants