Does this hold in general, i.e. also for deeper subfigure nestings?

My test has the axes on the second subfigure down.

Ok, I now see that this is technically correct: From #28170 (comment)

[for subfigures] I explicitly set self.figure = parent.figure which therefore resolves to the root figure, and I think that is because artists have things like self.figure.stale and self.figure.suppressComposite which would somehow have to magically thread themselves through.

But still needs some explanation, because it looks confusing: self._figure is the enclosing (sub)figure. and the _figure attribute of that is always the root figure. Maybe it helps to rename Artist._figure to Artist._enclosing_figure. Also, the second access is to a private attribute of (sub)figures, which general artists have no business with - we're relying on an implementation detail of a different class. Possibly a better spelling would be

I'd go with _root_figure to make the tree structure clear.

Oh OK, it never occurred to me that we shouldn't use private attributes on another class in the same package. But that means we shouldn't use self._enclosing_figure._root_figure so @jklymak have I misunderstood your comment?

@jklymak Am I correct here? Currently the _figure attribute semantics is

• _root_figure for subfigures
• _enclosing_figure for all other artists
• self for Figure

Yeah, I thought you meant the _root_figure by _enclosing_figure. Maybe call the artists' subfigure the _parent_figure or _parent_subfigure if we want to be more specific. I'd keep with standard tree terminology? https://en.wikipedia.org/wiki/Tree_(data_structure)#:~:text=A%20node%20that%20has%20a,same%20parent%20are%20sibling%20nodes.

I think _parent_figure as it may be either a Figure or a SubFigure. I note also that we already have SubFigure.parent, so it's good to be consistent with that.

I'm not 100% sure this is the best solution here, but it's based on the following considerations:

1. Since the parent figure is set at instantiation and Artist.set_figure does not allow switching to a different figure, (Sub)Figure.set_figure will always either be a no-op or raise an exception.
2. For consistency with Artist.set_figure, the no-op should be on the parent figure.
3. For backwards compatibility and consistency with the (Sub)Figure.figure property, the no-op should be on the root figure.

Not sure we can get rid of the setter without breaking OO principles (specifically Liskov's Substitution Principle)... And not sure it is worth working around that expectation break.

The parent implementation already raises if a figure has already been set.

It is a reasonable point that we already break the semantics of having a "set_figure" on a "Figure"... but on the other hand, all that has really changed is "a figure is set for you at instantiation", and it is actually consistent to say "set_figure on an artist that already has a figure is an error, here is a method that we just didn't override, it will error, but hey that is expected".

Not sure we can get rid of the setter without breaking OO principles (specifically Liskov's Substitution Principle)... And not sure it is worth working around that expectation break.

Liskov is already broken. You can’t use a Figure everywhere where you can use an Artist. If one wanted strict Liskov Substitution Principle, one would need a more refined inheritance architecture, which is not where we are. But I see this pragmatic: As long as nobody expects a functionality (you would not try figure.add_artist(figure)) or the behavior is reasonable (you can’t set another figure if one is already set), the API is good enough.

@ksunden can you advise what I should do here? It is a fact of the implementation that (Sub)Figure.figure can only ever be a Figure. It is also a fact that the setter can at best be a no-op, so it doesn't really make sense to me to have a type hint encouraging the user to set it. I can add the setter, but MyPy still complains about Figure being stricter than the parent's Union[Figure, SubFigure]. Adding this to the allowlist doesn't seem to make any difference. I'm stumped.

Firstly, the allow list is only for stubtest, and this is breaking a mypy check, so that is why that is not helping. A # type: ignore comment could be added, but that is a bit more of a last resort. (stubtest requires mypy to pass and runs it first, part of why it takes so long)

Secondly, is there a reason to be more strict here? Subfigures can be nested inside of subfigures, can they not? Type narrowing for a subclass isn't actually the problem here, but also not clear to me why it is narrowed here. (Not fully sure why you get two errors for each... but both are resolved below without adjusting their own type hints...)

Unfortunately, getting rid of the setter is breaking a pretty fundamental Object Orientation principle that all things that are provided by the parent class can be done on instances of child classes... and the method still exists, so I think type hinting it is fine...

That said, I would not be too opposed to making Artist.figure a read-only (type hinted at least) attribute, since I think writing to it (without using Artist.set_figure) is generally a mistake).

If I comment out the two lines from artist.pyi, I get no mypy issues found.

diff --git a/lib/matplotlib/artist.pyi b/lib/matplotlib/artist.pyi
index 08a99c0ed1..730c530dd2 100644
--- a/lib/matplotlib/artist.pyi
+++ b/lib/matplotlib/artist.pyi
@@ -33,8 +33,8 @@ class Artist:
     stale_callback: Callable[[Artist, bool], None] | None
     @property
     def figure(self) -> Figure | SubFigure: ...
-    @figure.setter
-    def figure(self, fig: Figure | SubFigure): ...
+    #@figure.setter
+    #def figure(self, fig: Figure | SubFigure): ...
     clipbox: BboxBase | None
     def __init__(self) -> None: ...
     def remove(self) -> None: ...

In implementation, Artist.figure is a standard instance attribute, made in __init__, so it is writable by anyone... but I'm good with making the type hint stricter than that (even without making the implementation enforce it)

Also, looking back... the type hint is technically wrong, as artist.figure can in fact be None... but that may just be a "useful fib", since 99% of artists that you have will in fact have a figure, and so checking for the none case in all usages is perhaps not something we wish to enforce with type hints...

Re type narrowing, I guess actually closely reading the code, you do narrow, so not opposed to encapsulating that meaning if that is what the code does... but do have some slight hesitations there, particularly if standard artists don't get root for art.figure... I'd have leaned towards not special casing Figure there...

is there a reason to be more strict here? Subfigures can be nested inside of subfigures, can they not?

Unfortunately we have a mismatch in what this property is: Artist.figure is the parent (Sub)Figure but SubFigure.figure is the root Figure. See #28170 (comment) for why it's difficult to change that.

[slow x-post]

#28170 (comment) suggested discouraging use of the figure attribute. Would it therefore be reasonable to just not type hint it? Looks like that would require a few updates in pyplot to use get_figure instead, but that's simple enough.

The motivation for moving from figure to get_figure() is clarity - do we want the parent or the root? Internally, we should not be using figure anymore. For now, it should only stay available for backward compatibility. Whether or not we want to deprecate it and force downstream users to migrate can be decided later.

I'm not clear what the effect of not type-hinting is. Would it not show up in completions? Would there be warnings on incomplete typing?

I'm (clearly!) no expert on type hinting but I took out the hint for this property in my branch and the mypy-precommit flagged failures in pyplot.

Internally, we should not be using figure anymore.

Should internal usage be removed in this PR or could it wait for a follow-up? I lean towards doing it separately as I think the current change stands by itself and should be useful for the subfigure_mosaic work (#28170 (comment)). I have started having a go at removing the internal use but there is, um, rather a lot of it and not every instance is obvious to me whether it should be root or not. So I think separating those concerns off into a separate PR may make the review easier.

Sure, the internal refactoring can be a follow-up.

and not every instance is obvious to me whether it should be root or not

That's part of the motivation to get rid of it 😏. I also won't guarantee that the current usages are always correct. Subfigures (and in particular nested subfigures) are fairly recent and thus not that common.