Skip to content
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

Deprecate {ContourSet,Quiver}.ax in favor of .axes. #16058

Merged
merged 1 commit into from Apr 13, 2020

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Dec 31, 2019

For consistency with other artists. In particular, Colorbar.remove
assumes that the .axes attribute exists.

Also a smattering of cleanups.

Edit: also do the same for ContourSet, per #12443 (comment).

PR Summary

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

@timhoffm
Copy link
Member

timhoffm commented Jan 1, 2020

The use of attributes .ax vs .axes is a mess. Instead of fixing single issues like here or in ContourSets in #12443, we need to

  1. Evaluate the current state
  2. Decide which one is the preferred solution.
  3. Consistently change it everywhere in the code base.

Step 1: Current state

Usages of self.ax (searched for self.ax = ):

  • Quiver
  • ColorbarBase
  • ContourSet
  • Sankey
  • AxesWidget
  • ToolHandles
  • axes_grid1.ColorbarBase

Usages of self.axes (searched for self.axes = ):

  • AxisArtist
  • Artist
  • _AxesBase (which is an Artist)
  • axis_grid1.Axes
  • Tick
  • Axis
  • _ImageBase
  • Legend
  • VertexSelector
  • _ThetaShift
  • Spine
  • MultiCursor
  • GridHelperRectlinear

Step 2: Preferred solution

Pro self.ax:

  • The canonical variable name is ax (fig, ax = plt.subplots()).
    • It's easy to guess/remember if you are familiar with matplotlib conventions.
    • That choice was deliberate to prevent plural naming issues when collections containing Axes.

Pro 'self.axes`:

  • More classes currently use this variant. But I'm not clear for which classes this attribute is accessed by users; so not sure deprecating ax would have the smaller impact on users.
  • The name is more readable for the uninformed user (but then again does he know what an axes is?).

IMHO, there's no clear winner. I slightly lean towards keeping self.ax. But this is really up for discussion.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 1, 2020

More classes currently use this variant.

Just to be clear, it's a lot more classes, in practice (not just 13 vs 7 like the count above suggests): all Artists have .axes, so that's also Line2D, all the Collections, all the Patches, etc... which are also much more commonly used than any of the classes that use .ax. And as a consequence, other classes (e.g. Colorbar, as noted in #15986 (comment), but also Animation) also typically assume that they can access parent axes of other objects (which are typically artists, but not always) with .axes.

@anntzer anntzer changed the title Deprecate Quiver.ax in favor of Quiver.axes. Deprecate {ContourSet,Quiver}.ax in favor of .axes. Mar 25, 2020
@efiring
Copy link
Member

efiring commented Apr 7, 2020

As the most likely guilty party in establishing the ax attributes, I'm OK with switching to axes. It will be interesting to see how much external code this ends up affecting.

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'm also fine with switching to axes.

@tacaswell since this change may possibly affect a number of users, IMHO you should also give your consent.

For consistency with other artists.  In particular, Colorbar.remove
assumes that the `.axes` attribute exists.

Also a smattering of cleanups.
@anntzer
Copy link
Contributor Author

anntzer commented Apr 12, 2020

rebased

@tacaswell tacaswell added this to the v3.3.0 milestone Apr 13, 2020
@tacaswell
Copy link
Member

As we don't have a good sense of how big of an impact this actually has I am in favor of merging this and seeing what push back we get in the rc / 3.3.0. We should definitly have the .axes alias to make the API more uniform and it would be better to have only one way to do this, but if we get too many complaints we should be ready to leave .ax permanently.

@tacaswell tacaswell merged commit 0824d00 into matplotlib:master Apr 13, 2020
@anntzer anntzer deleted the quiverax branch April 13, 2020 19:50
@QuLogic QuLogic removed the status: needs comment/discussion needs consensus on next step label Apr 13, 2020
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.

None yet

6 participants