Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

added typing_extensions.Self to _AxesBase.twinx#28625

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

randolf-scholz
Copy link
Contributor

PR summary

Copy link

@github-actionsgithub-actionsbot 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 uson gitter for real-time discussion.

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

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

@ksunden
Copy link
Member

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
ContributorAuthor

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 commentedJul 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 inpyi files (or guarded byif 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 aruntime dependency necessarily.

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

For twinx/twiny, the Axes that is returned is created byFigure.add_subplot orFigure.add_axes, which both returnAxes, specifically.

This means that if you calltwinx 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 isAxes here, notSelf:

Created here:

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:

@overload
defadd_axes(self,ax:Axes)->Axes: ...
@overload
defadd_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
defadd_subplot(
self,nrows:int,ncols:int,index:int|tuple[int,int],**kwargs
)->Axes: ...
@overload
defadd_subplot(self,pos:int,**kwargs)->Axes: ...
@overload
defadd_subplot(self,ax:Axes,**kwargs)->Axes: ...
@overload
defadd_subplot(self,ax:SubplotSpec,**kwargs)->Axes: ...
@overload
defadd_subplot(self,**kwargs)->Axes: ...

This generally falls into the category of_AxesBase is nottotally wrong...Axes is a subclass thereof, butSelf 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 thinkSelf may be the "ideal" type hint, just not actually reflective of the code as it stands...

@timhoffm
Copy link
Member

This means that if you calltwinx 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 toAxes.

@story645
Copy link
Member

story645 commentedAug 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.

@ksundenksunden added this to thev3.9.2 milestoneAug 1, 2024
@ksundenksunden merged commit7abb3ba intomatplotlib:mainAug 1, 2024
40 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestAug 1, 2024
@QuLogic
Copy link
Member

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 requestAug 1, 2024
…625-on-v3.9.xBackport PR#28625 on branch v3.9.x (added typing_extensions.Self to _AxesBase.twinx)
@randolf-scholzrandolf-scholz deleted the axesbase_twinx_type_hint branchAugust 2, 2024 08:02
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic left review comments

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

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Projects
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

[8]ページ先頭

©2009-2025 Movatter.jp