Skip to content

Correctly handle Axes subclasses that override cla #23735

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 26, 2022
Merged

Correctly handle Axes subclasses that override cla #23735

merged 3 commits into from
Aug 26, 2022

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Aug 25, 2022

PR Summary

In #22802, Axes.clear was made the primary interface over Axes.cla. However, while this can be enforced internally, that can't be said for third-party subclasses. I'm pretty sure we did not intend to remove or deprecate Axes.cla given its wide use, though I'm not sure if we wanted to discourage it (by undocumenting or just putting that in the docstring.)

This fixes, e.g., Cartopy, but probably most other third-party packages that will subclass Axes.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [n/a] New features are documented, with examples if plot related.
  • [n/a] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [n/a] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@QuLogic QuLogic added Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: axes labels Aug 25, 2022
@QuLogic QuLogic added this to the v3.6.0 milestone Aug 25, 2022
@QuLogic QuLogic mentioned this pull request Aug 25, 2022
Merged
@timhoffm
Copy link
Member

I'm pretty sure we did not intend to remove or deprecate Axes.cla given its wide use, though I'm not sure if we wanted to discourage it (by undocumenting or just putting that in the docstring.)

We cannot deprecate Axes.cla as an interface because it's too widely used. But I think we should discourage it (docstring), because clear() is much more clear 😜, and in general we should give guidance which way to use if we really need to have to ways of doing the same thing.

I tend to say that we also should nudge downstream libraries to override clear() instead of cla(). - i.e. put a pending deprecation in the init_sublcass "If you want to override clearing Axes, override "Axes.clear" instead of "Axes.cla". "Axes.cla" will only be an alias delegating to clear in the future, and thus standard clear operations will not call an overriden cla in the future."

@QuLogic
Correctly handle Axes subclasses that override cla
74b2c2d 
This fixes, e.g., Cartopy, but probably most other third-party packages
that will subclass `Axes`.
greglucas
greglucas approved these changes Aug 26, 2022
View reviewed changes
Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

This works as expected with Cartopy's various versions and emitting the PendingDeprecation. One thing that arises is the redefinition of _clear() in the subclasses. I wonder if that name is a bit special and also potentially used by other downstream libraries already and this would also get them into trouble?

timhoffm
timhoffm reviewed Aug 26, 2022
View reviewed changes
tacaswell and others added 2 commits August 26, 2022 14:05
@tacaswell @timhoffm
DOC: improve docstrings
6e0ba4c 
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@tacaswell
MNT: remane _clear -> __clear
f569d1a
@tacaswell
Copy link
Member

@QuLogic can merge this is happy with the last 2 commits.

@QuLogic QuLogic merged commit 01e916d into matplotlib:main Aug 26, 2022
@meeseeksmachine meeseeksmachine mentioned this pull request Aug 26, 2022
Merged
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Aug 26, 2022
@QuLogic @meeseeksmachine
Backport PR matplotlib#23735: Correctly handle Axes subclasses that o…
50b009a 
…verride cla
@QuLogic QuLogic deleted the fix-clear-cla branch August 26, 2022 19:48
QuLogic added a commit that referenced this pull request Aug 26, 2022
@QuLogic
Merge pull request #23749 from meeseeksmachine/auto-backport-of-pr-23…
7fa7db8 
…735-on-v3.6.x

Backport PR #23735 on branch v3.6.x (Correctly handle Axes subclasses that override cla)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dopplershift dopplershift dopplershift left review comments

@anntzer anntzer anntzer left review comments

@tacaswell tacaswell tacaswell approved these changes

@timhoffm timhoffm timhoffm approved these changes

@greglucas greglucas greglucas approved these changes

Assignees
No one assigned
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: axes
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.
6 participants
@QuLogic @timhoffm @tacaswell @dopplershift @anntzer @greglucas