8000 Propagate Axes class and kwargs for twinx and twiny by cmp0xff · Pull Request #29325 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Propagate Axes class and kwargs for twinx and twiny #29325

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 7 commits into from
Jan 8, 2025
Merged

Propagate Axes class and kwargs for twinx and twiny #29325

merged 7 commits into from
Jan 8, 2025

Conversation

cmp0xff
Copy link
Contributor
@cmp0xff cmp0xff commented Dec 16, 2024 •
edited
Loading

PR summary

Why is this change necessary?

Users may wish to inherit matplotlib.axes.Axes, see e.g. https://stackoverflow.com/q/48593124.

  • In such applications, twinx / twiny in the subclass should return subclass instances, instead of Axes.
  • If the subclass supports additional kwargs, twinx / twiny should also be able to accept these extra arguments.

What problem does it solve?

The proposed changes fix the behaviour of the methods mentioned above..

What is the reasoning for this implementation?

We try to keep the changes to a minimum and maintain current behaviours.

PR checklist

@cmp0xff cmp0xff marked this pull request as ready for review December 16, 2024 16:35
@cmp0xff cmp0xff force-pushed the feature/cmp0xff/axes-class-and-kwargs-for-twinx-and-twiny branch from 687849d to b1d6a54 Compare December 16, 2024 19:27
@timhoffm
Copy link
Member

I'm somewhat hesitant to do this. While it looks feasible at first glance, I've some reservations:

  • What is the use case? The examples in the stackoverflow link seem a bit far fetched. Are we really solving a problem with this, or are we just improving on how one can abuse the library?
  • twinx/twiny is currently limited to Axes. I'd be happy to simply document that limitation. Twinning is likely confusing or even not feasible on non-rectangular coordinate systems - polar plots are such a case. When opening up to other Axes subclasses, we allude that it works ("does the right thing") there, but I have absolutely no clue whether that'd be the case.

@cmp0xff
Copy link
Contributor Author
cmp0xff commented Dec 17, 2024 •
edited
Loading

Hi @timhoffm , thank you for the comments.

Twinning is powerful for time serieses with different units

Twinning is likely confusing or even not feasible on non-rectangular coordinate systems - polar plots are such a case.

When plotting time series data, it happens that several serieses with different units are to be shown in a single figure, with which users would like to interact. One can use the time labels as the shared x axis to twin two overlapping subplots, each has its own y axis with its own unit. This enables simultaneous interaction on the subplots; paning and zooming in one subplot act in the same way on the shared axes of both subplots.

Subclassing Axes

Are we really solving a problem with this, or are we just improving on how one can abuse the library?

Being able to subclass Axes is both a potential for the user, AND a feature of matplotlib.

  • The existence of axes_class as a public argument in Figure.add_subplot and Figure.add_axes already shows this.
  • In mpl_toolkits.axisartist, we have an example of subclassing Axes.
  • In _AxesBase._make_twin_axes, which is only used by twinx and twiny, we already have the potential to insert *args and **kwargs. However they were not in use before the current MR.

@rcomer
Copy link
Member
rcomer commented Dec 17, 2024 •
edited
Loading

@cmp0xff could you give an example of a use-case where you need both a subclass and a twinned axes? I appreciate the value of subclasses and I have added functionality to Matplotlib to handle them (#22608). For that PR I had a specific subclass in mind (the Cartopy GeoAxes) as shown in the PR summary. Do you have a specific subclass in mind for this?

@cmp0xff cmp0xff force-pushed the feature/cmp0xff/axes-class-and-kwargs-for-twinx-and-twiny branch from b1d6a54 to 2feddad Compare December 17, 2024 21:14
@cmp0xff
Copy link
Contributor Author
cmp0xff commented Dec 19, 2024

Hi @rcomer , thank you for your hints. After some research, I have realised that what I described above has mostly been implemented by parasite_axes in the axisartist toolkit.

In practise, we have an in-house extension of matplotlib.axes.Axes which also implements resampling, where twinx is also needed. However, the function returns an Axes instance, instead of instantiating the extended Axes. Therefore the whole twinx function needs to be reimplemented, which is almost entirely the same with the original twinx, with the single addition that the extended Axes class is returned.

Now that in 3.10.0, twinx has been updated, we need to propagate the changes to our reimplementation. What if the original twinx could be adapted a bit, so that it could become more flexible and handle subclasses? This was the motivation of the current MR.

@timhoffm
Copy link
Member

Thanks, this sounds like a reasonable application. I'm fundamentally ok with extending/modifying the behavior of twinx().

Design considerations:

  • It may make sense to twin an Axes subclass of another type, e.g. you could twin your ResamplingAxes to a regular Axes. This would require an explicit axes_class parameter: ax.twinx(axes_class=ResamplingAxes). This should be made explicit and not folded into kwargs. A corollary is that the return type would stay Axes (or the signature must be overloaded).
  • If we take an axes_class parameter, should the default be Axes (current behavior, maximal compatibility) or Self (possibly slightly more common application)?

cmp0xff added a commit to cmp0xff/matplotlib that referenced this pull request Dec 20, 2024
@cmp0xff
fix(comment): matplotlib#29325 (comment)
a9c8ca4
@cmp0xff cmp0xff force-pushed the feature/cmp0xff/axes-class-and-kwargs-for-twinx-and-twiny branch from 2feddad to a9c8ca4 Compare December 20, 2024 23:12
cmp0xff added a commit to cmp0xff/matplotlib that referenced this pull request Dec 20, 2024
@cmp0xff
fix(comment): matplotlib#29325 (comment)
b337f94
@cmp0xff cmp0xff force-pushed the feature/cmp0xff/axes-class-and-kwargs-for-twinx-and-twiny branch from a9c8ca4 to b337f94 Compare December 20, 2024 23:13
cmp0xff added a commit to cmp0xff/matplotlib that referenced this pull request Dec 21, 2024
@cmp0xff
fix(comment): matplotlib#29325 (comment)
c4c3aa7
@cmp0xff cmp0xff force-pushed the feature/cmp0xff/axes-class-and-kwargs-for-twinx-and-twiny branch from b337f94 to a228f5d Compare December 21, 2024 10:28
@cmp0xff
Copy link
Contributor Author
cmp0xff commented Dec 21, 2024

Hi @timhoffm ,

It may make sense to twin an Axes subclass of another type, e.g. you could twin your ResamplingAxes to a regular Axes. This would require an explicit axes_class parameter: ax.twinx(axes_class=ResamplingAxes). This should be made explicit and not folded into kwargs. A corollary is that the return type would stay Axes (or the signature must be overloaded).

c4c3aa7

If we take an axes_class parameter, should the default be Axes (current behavior, maximal compatibility) or Self (possibly slightly more common application)?

After some thinking, I would go for keeping the current behaviour, defaulting to None (rather than self).

The reason is that some subclasses use projection or polar to instantiate themselves; these parameters are incompatible with axes_class, which should hence stay None for these subclasses.

timhoffm
timhoffm reviewed Dec 21, 2024
View reviewed changes
cmp0xff added a commit to cmp0xff/matplotlib that referenced this pull request Dec 21, 2024
@cmp0xff
fix(comment): matplotlib#29325 (comment)
3989225
@cmp0xff cmp0xff force-pushed the feature/cmp0xff/axes-class-and-kwargs-for-twinx-and-twiny branch from de198d2 to ecb384e Compare December 21, 2024 22:55
@cmp0xff cmp0xff force-pushed the feature/cmp0xff/axes-class-and-kwargs-for-twinx-and-twiny branch from 3b117c0 to 18589f3 Compare December 27, 2024 10:38
@cmp0xff
Copy link
Contributor Author
cmp0xff commented Jan 2, 2025

Hi, it has been two weeks since the last update. May I ask for a second approval / a critical review? Thanks and happy new year.

rcomer
rcomer reviewed Jan 4, 2025
View reviewed changes
Copy link
Member
@rcomer rcomer left a comment

Choose a reason for hiding this comment

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

Thanks @cmp0xff, I just have some comments on the docstrings.

cmp0xff added a commit to cmp0xff/matplotlib that referenced this pull request Jan 8, 2025
@cmp0xff @rcomer
Apply suggestions from code review matplotlib#29325 (comment) matplot…
883f049 
…lib#29325 (comment)

Co-authored-by: Ruth Comer <10599679+rcomer@users.noreply.github.com>
@cmp0xff cmp0xff force-pushed the feature/cmp0xff/axes-class-and-kwargs-for-twinx-and-twiny branch from df825b1 to 883f049 Compare January 8, 2025 10:39
@rcomer rcomer added this to the v3.11.0 milestone Jan 8, 2025
@cmp0xff @rcomer
Apply suggestions from code review
bb8e374 
- matplotlib#29325 (comment)
- matplotlib#29325 (comment)
- matplotlib#29325 (comment)

Co-authored-by: Ruth Comer <10599679+rcomer@users.noreply.github.com>
@cmp0xff cmp0xff force-pushed the feature/cmp0xff/axes-class-and-kwargs-for-twinx-and-twiny branch from 883f049 to bb8e374 Compare January 8, 2025 10:59
@rcomer rcomer merged commit 8d3c4db into matplotlib:main Jan 8, 2025
39 checks passed
@cmp0xff cmp0xff deleted the feature/cmp0xff/axes-class-and-kwargs-for-twinx-and-twiny branch January 8, 2025 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timhoffm timhoffm timhoffm approved these changes

@rcomer rcomer rcomer approved these changes

Assignees
No one assigned
Labels
New feature topic: axes
Projects
None yet
Milestone
v3.11.0
Development

Successfully merging this pull request may close these issues.
3 participants
@cmp0xff @timhoffm @rcomer
0