8000 added typing_extensions.Self to _AxesBase.twinx by randolf-scholz · Pull Request #28625 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

added typing_extensions.Self to _AxesBase.twinx #28625

8000 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 5 commits into from
Aug 1, 2024
Merged

added typing_extensions.Self to _AxesBase.twinx #28625

merged 5 commits into from
Aug 1, 2024

Conversation

randolf-scholz
Copy link
Contributor

PR summary

@github-actions github-actions bot added 8000 the topic: axes label Jul 29, 2024
@randolf-scholz randolf-scholz mentioned this pull request Jul 29, 2024
Closed
github-actions[bot]
github-actions bot reviewed Jul 29, 2024
View reviewed changes
Copy link
@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

QuLogic
QuLogic reviewed Jul 29, 2024
View reviewed changes
@ksunden
Copy link
Member
ksunden commented Jul 29, 2024

Additionally, we are now python 3.10+ on main anyway, so not extensions is not actually needed for Self.

Note however that to be backported to the next micro release, we would need to take care there, since it still runs on py 3.9.

@randolf-scholz
Copy link
Contributor Author

Additionally, we are now python 3.10+ on main anyway, so not extensions is not actually needed for Self

typing.Self only became part of the standard library in 3.11

@ksunden
Copy link
Member
ksunden commented Jul 31, 2024 •
edited
Loading

Ah, I remembered Self coming in (at least added in some places) with recent changes (#28503) that were broadly related to py 3.10 being the new minimum.

Regardless, relying on typing_extensions in pyi files (or guarded by if TYPE_CHECKING) is not new here, though perhaps we could be more explicit about it somewhere.

I do not think we want to make it a runtime dependency necessarily.

All that said, in this instance, I don't actually think Self describes what actually happens...

For twinx/twiny, the Axes that is returned is created by Figure.add_subplot or Figure.add_axes, which both return Axes, specifically.

This means that if you call twinx on a Polar plot (or geographic projection or 3D plot, etc) you get a "normal" axes back... (See #12506).

Thus the actual correct type hint is Axes here, not Self:

Created here:

matplotlib/lib/matplotlib/axes/_base.py

Lines 4522 to 4525 in 44be14c

twin = self.figure.add_subplot(ss, *args, **kwargs)
else:
twin = self.figure.add_axes(
self.get_position(True), *args, **kwargs,

Type hints for those methods:

matplotlib/lib/matplotlib/figure.pyi

Lines 70 to 93 in 44be14c

@overload
def add_axes(self, ax: Axes) -> Axes: ...
@overload
def add_axes(
self,
rect: tuple[float, float, float, float],
projection: None | str = ...,
polar: bool = ...,
**kwargs
) -> Axes: ...
# TODO: docstring indicates SubplotSpec a valid arg, but none of the listed signatures appear to be that
@overload
def add_subplot(
self, nrows: int, ncols: int, index: int | tuple[int, int], **kwargs
) -> Axes: ...
@overload
def add_subplot(self, pos: int, **kwargs) -> Axes: ...
@overload
def add_subplot(self, ax: Axes, **kwargs) -> Axes: ...
@overload
def add_subplot(self, ax: SubplotSpec, **kwargs) -> Axes: ...
@overload
def add_subplot(self, **kwargs) -> Axes: ...

This generally falls into the category of _AxesBase is not totally wrong... Axes is a subclass thereof, but Self is also not correct when you go further down the inheritance tree. That leaves us with a weird circular reference problem, but I don't think that should actually be a problem in pyi files.

However... I think Self may be the "ideal" type hint, just not actually reflective of the code as it stands...

@timhoffm
Copy link
Member
timhoffm commented Aug 1, 2024

This means that if you call twinx on a Polar plot (or geographic projection or 3D plot, etc)

It's generally not clear what twinning should mean on arbitrary axes or that it's useful. For example, where should the second radial axis on polar be placed? Polar does not even have a second radial axis. While it would be technically feasible for 3d and geo axis, the resulting plot is likely very difficult to interpret.

For now, I'd pragmatically say that we just don't provide proper twinning for these niche cases and set the return type to Axes.

@story645
Copy link
Member
story645 commented Aug 1, 2024 •
edited
Loading

While it would be technically feasible for 3d and geo axis, the resulting plot is likely very difficult to interpret

Um this has me realizing that a twinx geoAxis that shows the 0-360 convention and the 180W-180E convention could be useful for teaching both 😅

But also that's functionality that probably could live in cartopy if anyone wanted it.

@ksunden ksunden added this to the v3.9.2 milestone Aug 1, 2024
@ksunden ksunden merged commit 7abb3ba into matplotlib:main Aug 1, 2024
40 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Aug 1, 2024
@ksunden @meeseeksmachine
Backport PR matplotlib#28625: added typing_extensions.Self to _AxesBa…
7457ba4 
…se.twinx
@meeseeksmachine meeseeksmachine mentioned this pull request Aug 1, 2024
Merged
@QuLogic
Copy link
Member
QuLogic commented Aug 1, 2024

Thanks @randolf-scholz! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.

timhoffm added a commit that referenced this pull request Aug 1, 2024
@timhoffm
Merge pull request #28636 from meeseeksmachine/auto-backport-of-pr-28…
eb4b608 
…625-on-v3.9.x

Backport PR #28625 on branch v3.9.x (added typing_extensions.Self to _AxesBase.twinx)
@randolf-scholz randolf-scholz deleted the axesbase_twinx_type_hint branch August 2, 2024 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
topic: axes
Projects
First Time Contributors
Status: Merged
Milestone
v3.9.2
Development

Successfully merging this pull request may close these issues.

[Bug]: Bad type hint in _AxesBase.twinx()
5 participants
@randolf-scholz @ksunden @timhoffm @story645 @QuLogic
0