Missing return type hints for Figure

[ksunden]

PR Summary

Required changes for Figure.figure type hint

The core change here, which enables the plt.figure type hint is that
FigureBase.figure (and subclasses) is now type hinted as Figure instead of
FigureBase.

This is itself predicated on the fact that FigureBase is never directly instantiated.
Internally, this holds true, and so I'm inclined to think that is the case unless
proven otherwise.
Even our docs for Custom Figure subclasses
subclass from Figure, not FigureBase.
I will note that FigureBase refers to attributes only defined in the two subclasses:
Figure and SubFigure. (Namely at least self.patch and self.bbox, may be others)
This to me begs the question of whether FigureBase should 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)

• Decided on Figure instead of FigureBase
  • FigureBase never instantiated directly (If this assumption is not true, please speak up)
  • SubFigure.figure is always parent.figure
  • Figure.figure is always self
  • Therefore, while SubFigures can be nested arbitrarily, SubFigure.figure is always Figure (Given FigureBase not instantiated directly)
  • Plus it is what most users would expect, I'd think
• I think the case of None was me copying the axes.Figure was not actually possible
  • Not sure where I thought that was possible, but looking again, not possible

PR Checklist

Documentation and Tests

• Has pytest style unit tests (and pytest passes)
• Documentation is sphinx and numpydoc compliant (the docs should build without error).
• New plotting related features are documented with examples.

Release Notes

• New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
• API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
• Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

[tacaswell]

diff --git a/lib/matplotlib/figure.py b/lib/matplotlib/figure.py
index 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 FigureBase should be abstract (but not sure we want to go down concretely making it an abstract base class). I think it is clearer if each of the concrete sub-classes assign self.figure in their init rather than sub-figure re-assigning it. I think this also reduces the slight of hand here?

[ksunden]

There is a slight complexity with that doesn't actually come to light in the current code:

I did figure out that the None behavior was from copying from Artist.figure, which can be None. So removing self.figure = self from FigureBase actually just inherits that type hint/default of None.

I was worried that that would mean our if isinstance(num, FigureBase): in plt.figure would then have that type hint, but we are actually saved because the type hint on num itself is:

num: int | str | Figure | SubFigure | None = None,

So the only valid types that pass that isinstance are Figure and SubFigure, which both have the type of .figure narrowed from FigureBase | None (the artist type hint) to Figure (which is a valid, narrower, type). Since num for that code path must follow both the initial type hint and the isinstance check, the type checker does in fact know that None is no longer valid.

It is still a little tricky in ways I don't fully like, but in ways that can likely be mostly ignored for user code.
It is actually guaranteed that the parent of a SubFigure is a Figure or Subfigure (i.e. not just "any FigureBase) by the extant type hint and docstring documented expectation.
So the type narrowing by isinstance mixing with the input type is the only real "trick" left.

The full diff (including stubs) is then:

diff --git a/lib/matplotlib/figure.py b/lib/matplotlib/figure.py
index 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.pyi
index 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: Bbox
diff --git a/lib/matplotlib/pyplot.py b/lib/matplotlib/pyplot.py
index 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.

Artist.figure then is the only use of FigureBase (besides inheritance) in the type hints (pyi stubs or pyplot hints specifically, the isinstance still checks it).

[ksunden]

Cycling to restart CI, as it was failing for unrelated reasons (jupyter) when this was opened. If it is still affected, I'll rebase

[tacaswell]

Can we adjust the type on Arist.figure as well?

[ksunden]

Rebased to (not quite, dodging #25782) main, updated Artist.figure type hint to Figure | SubFigure | None

[QuLogic]

Why did this change?

[ksunden]

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 like Literal[True] and Literal[False] should actually span bool, they technically do not, as they only handle the case of literals being passed, not a variable with type bool.

That said, in many cases it doesn't really make sense to pass anything other than a literal there, so the "passed an unknown bool" case is pretty unexpected.
e.g. the cbook.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 with overloads to mete out the case which is passed sqeeze=True with Literal[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 use Any (if overloads cannot properly distinguish the return type). This is actually what I've already done with pyplot.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 of squeeze here.