FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Deprecate {ContourSet,Quiver}.ax in favor of .axes. by anntzer · Pull Request #16058 · matplotlib/matplotlib · GitHub

Repository navigation

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

Merged
tacaswell merged 1 commit into
matplotlib:masterfrom
anntzer:quiverax
Apr 13, 2020
Merged

tacaswell merged 1 commit into
matplotlib:masterfrom
anntzer:quiverax

Conversation

anntzer commented Dec 31, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Jan 1, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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.

timhoffm added API: changes Changes to the public API, typically requiring deprecation. API: consistency Consistency of the matplotlib API, including naming, behavior, defaults, … status: needs comment/discussion needs consensus on next step labels Jan 1, 2020

anntzer commented Jan 1, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 changed the title Deprecate Quiver.ax in favor of Quiver.axes. Deprecate {ContourSet,Quiver}.ax in favor of .axes. Mar 25, 2020

efiring commented Apr 7, 2020

Copy link
Copy Markdown
Member

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.

timhoffm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Apr 12, 2020

Copy link
Copy Markdown
Contributor Author

rebased

tacaswell added this to the v3.3.0 milestone Apr 13, 2020

Copy link
Copy Markdown
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 merged commit 0824d00 into matplotlib:master Apr 13, 2020
anntzer deleted the quiverax branch April 13, 2020 19:50
QuLogic removed the status: needs comment/discussion needs consensus on next step label Apr 13, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API: changes Changes to the public API, typically requiring deprecation. API: consistency Consistency of the matplotlib API, including naming, behavior, defaults, …

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL