Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Missing return type hints for Figure#25607
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
diff --git a/lib/matplotlib/figure.py b/lib/matplotlib/figure.pyindex 926944e5a5..c698443e88 100644--- a/lib/matplotlib/figure.py+++ b/lib/matplotlib/figure.py@@ -188,7 +188,6 @@ class FigureBase(Artist): # axis._get_tick_boxes_siblings self._align_label_groups = {"x": cbook.Grouper(), "y": cbook.Grouper()}- self.figure = self self._localaxes = [] # track all axes self.artists = [] self.lines = []@@ -2454,6 +2453,7 @@ None}, default: None %(Figure:kwdoc)s """ super().__init__(**kwargs)+ self.figure = self self._layout_engine = None if layout is not None: Should we add this diff to this commit as well? I think I agree |
There is a slight complexity with that doesn't actually come to light in the current code: I did figure out that the I was worried that that would mean our num:int|str|Figure|SubFigure|None=None, So the only valid types that pass that isinstance are It is still a little tricky in ways I don'tfully like, but in ways that can likely be mostly ignored for user code. The full diff (including stubs) is then: diff --git a/lib/matplotlib/figure.py b/lib/matplotlib/figure.pyindex 926944e5a5..c698443e88 100644--- a/lib/matplotlib/figure.py+++ b/lib/matplotlib/figure.py@@ -188,7 +188,6 @@ class FigureBase(Artist): # axis._get_tick_boxes_siblings self._align_label_groups = {"x": cbook.Grouper(), "y": cbook.Grouper()}- self.figure = self self._localaxes = [] # track all axes self.artists = [] self.lines = []@@ -2454,6 +2453,7 @@ None}, default: None %(Figure:kwdoc)s """ super().__init__(**kwargs)+ self.figure = self self._layout_engine = None if layout is not None:diff --git a/lib/matplotlib/figure.pyi b/lib/matplotlib/figure.pyiindex 2b9f4584e4..15299c2966 100644--- a/lib/matplotlib/figure.pyi+++ b/lib/matplotlib/figure.pyi@@ -70,10 +70,6 @@ class SubplotParams: ) -> None: ... class FigureBase(Artist):- # Small slight-of-hand as FigureBase is not instantiated directly- # hinting as Figure means e.g. plt.figure can be type hinted more easily- # FigureBase is never instantiated directly in internal code at the very least- figure: Figure artists: list[Artist] lines: list[Line2D] patches: list[Patch]@@ -296,6 +292,7 @@ class SubFigure(FigureBase): def get_axes(self) -> list[Axes]: ... class Figure(FigureBase):+ figure: Figure bbox_inches: Bbox dpi_scale_trans: Affine2D bbox: Bboxdiff --git a/lib/matplotlib/pyplot.py b/lib/matplotlib/pyplot.pyindex 2933fa5d4c..ace36725e3 100644--- a/lib/matplotlib/pyplot.py+++ b/lib/matplotlib/pyplot.py@@ -858,6 +858,7 @@ default: None in the matplotlibrc file. """ if isinstance(num, FigureBase):+ # type narrowed to `Figure | SubFigure` by combination of input and isinstance if num.canvas.manager is None: raise ValueError("The passed figure is not managed by pyplot") _pylab_helpers.Gcf.set_active(num.canvas.manager) I think I agree, and will add it.
|
Cycling to restart CI, as it was failing for unrelated reasons (jupyter) when this was opened. If it is still affected, I'll rebase |
Can we adjust the type on |
Required changes for Figure.figure type hint
Rebased to (not quite, dodging#25782) main, updated |
| @@ -154,15 +153,15 @@ class FigureBase(Artist): | |||
| *, | |||
| sharex: bool | Literal["none", "all", "row", "col"] = ..., | |||
| sharey: bool | Literal["none", "all", "row", "col"] = ..., | |||
| squeeze:Literal[True] = ..., | |||
| squeeze:bool = ..., | |||
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.
Why did this change?
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.
This is a particular case of a potentially more general issue, which was closely related enough to this change that I decided to make the change for this one here.
Essentially while it seems likeLiteral[True] andLiteral[False] should actually spanbool, they technically do not, as they only handle the case of literals being passed, not a variable with typebool.
That said, in many cases it doesn't really makesense to pass anything other than a literal there, so the "passed an unknown bool" case is pretty unexpected.
e.g. thecbook.load_data method has a couple booleans that we would never expect to be passed as an unknown bool (in part because that is a largely for internal use method).
In this particular case a more complete answer would be to try and do our best withoverloads to mete out the case which is passedsqeeze=True withLiteral[1] for nrows/ncols and then have a fallback for this version which can't necessarily differentiate what the return will be.
In fact, unions in returns are generally discouraged and the prevailing wisdom is to just useAny (if overloads cannot properly distinguish the return type). This is actually what I've already done withpyplot.subplots, which calls this method.
Removing such unions is a change I've been leaning towards doing at some point, but made a minimal change here as the return union actually encompasses all possible returns regardless of the value ofsqueeze here.
PR Summary
Required changes for Figure.figure type hint
The core change here, which enables the
plt.figuretype hint is thatFigureBase.figure(and subclasses) is now type hinted asFigureinstead ofFigureBase.This is itself predicated on the fact that
FigureBaseis never directly instantiated.Internally, this holds true, and so I'm inclined to think that is the case unless
proven otherwise.
Even our docs forCustom Figure subclasses
subclass from
Figure, notFigureBase.I will note that FigureBase refers to attributes only defined in the two subclasses:
FigureandSubFigure. (Namely at leastself.patchandself.bbox, may be others)This to me begs the question of whether
FigureBaseshould be an Abstract Base Class,thus defining methods that any valid subclass should have (including those attributes,
which may be required to become properties if so, as I don't think ABC can mark
pure attributes as abstract)
See discussion:#24976 (comment)
Figureinstead ofFigureBaseFigureBasenever instantiated directly (If this assumption is not true, please speak up)SubFigure.figureis alwaysparent.figureFigure.figureis alwaysselfSubFigure.figureis alwaysFigure(GivenFigureBasenot instantiated directly)Nonewas me copying theaxes.Figurewas not actually possiblePR Checklist
Documentation and Tests
pytestpasses)Release Notes
.. versionadded::directive in the docstring and documented indoc/users/next_whats_new/.. versionchanged::directive in the docstring and documented indoc/api/next_api_changes/next_whats_new/README.rstornext_api_changes/README.rst