-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Simplify Tick and Axis initialization. #14948
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
Conversation
55ef2b9
to
f4355df
Compare
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. |
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
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
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. |
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.
@timhoffm I added a proper deprecation warning / deprecation period in case anyone was actually relying on overriding these methods. |
There was a problem hiding this 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.
- matplotlib/matplotlib#14948 - matplotlib/matplotlib@SHA c3f87d4afa3e1d0f573322eaaa06d5377328a0b5
- matplotlib/matplotlib#14948 - matplotlib/matplotlib@SHA: c3f87d4afa3e1d0f573322eaaa06d5377328a0b5
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