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

[TYP] Type changes from running against Pandas#26883

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
timhoffm merged 3 commits intomatplotlib:mainfromksunden:generic_artistlist
Oct 7, 2023

Conversation

ksunden
Copy link
Member

@ksundenksunden commentedSep 22, 2023
edited
Loading

PR summary

This is a few typing related changes identified by running against Pandas

  • MakingArtistList generic so that Axes properties can be type narrowed
  • Adding missed attributes to Axes
    • These are added in__clear, rather than__init__ directly, so they are available on all Axes, but were missed when looking for attrs
  • Add a__all__ toaxes/__init__.pyi
  • Edit howaxes/__init__.pyi handles imports/re-exportingSubplot
    • Only added to the pyi stub, not runtime
    • Subplot was not recognized as exported because it was not a redundant import
  • Add_AxisWrapper to type hint forTickHelper.set_axis
    • A more extensive fix for this may be warranted, as we are leaking private classes in the type hint
    • AxisWrapper is set in Polar code and flags if absent here
    • this just ensures thatx.set_axis(x.axis) type checks as OK
    • More extensive approach would perhaps to use a Protocol class to dynamically ducktype rather than having concrete types here
      • Or a type alias (either to the union that includes those or toAny

PR checklist

@ksunden
Copy link
MemberAuthor

Apparently stubtest doesn't like that I put__all__ only in the pyi... I was trying to avoid changing the.py file here because I kind of want to backport it to 3.8.1... and would prefer not to make that change to py/from matplotlib.axes import * behavior in a patch release (I'm relatively okay with doing it for stubs, especially since thosedon't export all imports by default)

@@ -1,10 +1,12 @@
from typing import TypeVar

from ._axes import *
from ._axes import Axes as Subplot
from ._axes import Axes as Axes
Copy link
Member

Choose a reason for hiding this comment

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

Since that's the only thing in there, should we change the import in the.py file, too?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I mean, logically yes, but technically at runtime more gets imported there, just not things weintend to re-export.

I was being a bit conservative here andonly changing stub behavior (especially since the stubsdidn't pick upSubplot as being exported, when itshould be)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I actually think__init__.py files (broadly) are good candidates for inlining type hints and just not having separate files: they are short, contain little to no actual function definitions, it's mostly imports and__all__ (which I'm also in favor of adding in more places)

Comment on lines +59 to +61
containers: list[Container]
callbacks: CallbackRegistry
child_axes: list[_AxesBase]
Copy link
Member

Choose a reason for hiding this comment

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

Looking at__clear, I see alsolegend_ andtitle that aren't here; should they be added as well? (The former appears inaxes.pyi onAxes)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I'd say those ones (and perhaps several of the existing attrs) should be made private (title there is already_[left|right]_title whichare private, though the getters/setters are defined in_axes.py, not_base.py, for some reason, and not defined forsecondary_axes, even though the attrs exist)

But I'll add them since they are by naming convention public (though I will also note that none of these attrs (aside fromdataLim andviewLim) actually appear in the rendered docs, though several (trans[Axes,Scale,Limits,Data] come to mind)are referred to by examples, etc. Is that something wewant to include in the rendered docs? cc@timhoffm, this is reminiscent of the discussions regardingplt.Axes/plt.Figure, but is different in that these are not imports and are instance attributes)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

ax.title is used in several tests as well as thelifecycle tutorial andtight_layout_guide

axes.legend_ is used outside of theaxes module only inbackend/qt_editor/figureoptions.py as far as I can tell through quick greps (and never in documentation)

Copy link
Member

@QuLogicQuLogic left a comment

Choose a reason for hiding this comment

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

LGTM, if@timhoffm is good with the additional items.

timhoffm reacted with thumbs up emoji
Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

The instance attributes are technically public. Documenting the types is consistent with our current public API. It’s a separate question, whether they should be public, and IMHO many shouldn’t. But that has to go through the deprecation cycle.

@timhoffmtimhoffm added this to thev3.8.1 milestoneOct 7, 2023
@timhoffmtimhoffm merged commitd908b4f intomatplotlib:mainOct 7, 2023
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestOct 7, 2023
QuLogic added a commit that referenced this pull requestOct 7, 2023
…883-on-v3.8.xBackport PR#26883 on branch v3.8.x ([TYP] Type changes from running against Pandas)
@ksundenksunden mentioned this pull requestNov 2, 2023
5 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.8.1
Development

Successfully merging this pull request may close these issues.

3 participants
@ksunden@QuLogic@timhoffm

[8]ページ先頭

©2009-2025 Movatter.jp