Skip to content

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jul 31, 2019

Instead of delegating to a bunch of private methods (_get_text1,
_get_text2, _get_tick1line, _get_tick2line, _get_gridline, _get_label,
_get_offset_text) to construct the sub-artists (lines, labels, etc.) of
Ticks and Axises, just set up some default instances in the base class
constructor and make additional customizations in the subclass
constructor.

A quick grepping suggests (no guarantees) that neither cartopy nor
wcsaxes override these private methods (an API where third-party
subclasses are encouraged to override private methods is a bit
awkward...) and in any case the approach of customizing the sub-artists
in the subclass' __init__ works with all versions of Matplotlib.

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

@anntzer anntzer force-pushed the axisinit branch 2 times, most recently from 55ef2b9 to f4355df Compare August 5, 2019 07:24
@timhoffm
Copy link
Member

I'm -0.2 on this change. It does not not greatly simplify the initialization. This splits the initialization into base and subclass parts, which reduces redundancy between x and y, but OTOH splits the initialization logic into two places (base and sub-class).

There's a theoretical advantage that subclasses do not have to override private methods. But we might be breakling third party packages that rely on the current behavior.

The first few f-stringifications could be merged separately.

@anntzer
Copy link
Contributor Author

anntzer commented Aug 12, 2019

It does not not greatly simplify the initialization. This splits the initialization into base and subclass parts, which reduces redundancy between x and y, but OTOH splits the initialization logic into two places (base and sub-class).

Previously the initialization was split into even more places -- the various _get_foo methods also contained that logic. In fact, given an Axis subclass, to follow its initialization you needed to walk its entire MRO and look up each _get_foo override. With this PR, you still need to walk the entire MRO, but only need to check overrides in __init__. So I personally think that this PR does simplify Axis initialization.

There's a theoretical advantage that subclasses do not have to override private methods. But we might be breakling third party packages that rely on the current behavior.

As mentioned above, a quick check (no exhaustivity guarantees) of the main Axis subclass implementors that I'm aware of, cartopy and wcsaxes (and basemap as well), reveals no such overrides. We could add a deprecation warning if we detect that a subclass does provide these methods, but I intentionally chose not to because it would become tricky to write code that works without warnings for all versions of matplotlib because compat with old matplotlib requires these methods to be overridden. With the current PR, if you want to keep your axis subclass compatible with all versions of mpl, you "just" need to duplicate the logic between _get_foo and __init__ (for example by directly calling _get_foo from the subclass' __init__.

The first few f-stringifications could be merged separately.

I can open a separate PR for that if this one gets stalled, but I would prefer not as this will lead to merge conflicts.

@tacaswell tacaswell added this to the v3.3.0 milestone Aug 12, 2019
Instead of delegating to a bunch of private methods (_get_text1,
_get_text2, _get_tick1line, _get_tick2line, _get_gridline, _get_label,
_get_offset_text) to construct the sub-artists (lines, labels, etc.) of
Ticks and Axises, just set up some default instances in the base class
constructor and make additional customizations in the subclass
constructor.

A quick grepping suggests (no guarantees) that neither cartopy nor
wcsaxes override these private methods (an API where third-party
subclasses are encouraged to override private methods is a bit
awkward...) and in any case the approach of customizing the sub-artists
in the subclass' `__init__` works with all versions of Matplotlib.
@anntzer
Copy link
Contributor Author

anntzer commented Nov 11, 2019

@timhoffm I added a proper deprecation warning / deprecation period in case anyone was actually relying on overriding these methods.

@jklymak jklymak requested a review from efiring November 17, 2019 19:47
Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

I think this is a net simplification, and I don't see any problems. Let's try it.

@jklymak jklymak merged commit c864d95 into matplotlib:master Nov 17, 2019
@anntzer anntzer deleted the axisinit branch November 17, 2019 22:54
yuzie007 added a commit to yuzie007/mpltern that referenced this pull request Jul 18, 2020
- matplotlib/matplotlib#14948
- matplotlib/matplotlib@SHA c3f87d4afa3e1d0f573322eaaa06d5377328a0b5
yuzie007 added a commit to yuzie007/mpltern that referenced this pull request Jul 18, 2020
- matplotlib/matplotlib#14948
- matplotlib/matplotlib@SHA: c3f87d4afa3e1d0f573322eaaa06d5377328a0b5
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.

5 participants