Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Doc]: get_figure may return a SubFigure #28170

Open
rcomer opened this issue May 4, 2024 · 7 comments · May be fixed by #28177
Open

[Doc]: get_figure may return a SubFigure #28170

rcomer opened this issue May 4, 2024 · 7 comments · May be fixed by #28177

Comments

@rcomer
Copy link
Member

rcomer commented May 4, 2024

Documentation Link

https://matplotlib.org/devdocs/api/_as_gen/matplotlib.artist.Artist.get_figure.html#matplotlib.artist.Artist.get_figure

Problem

I'm not sure whether this is a documentation issue or a bug. The docstring for get_figure states that it returns a Figure, which I think implies the parent figure. If our artist is on a subfigure, it will return that. Note that SubFigure does not inherit from Figure.

In [1]: import matplotlib.pyplot as plt

In [2]: fig = plt.figure()

In [3]: sfig = fig.subfigures()

In [4]: ax = sfig.subplots()

In [5]: ax.get_figure()
Out[5]: <matplotlib.figure.SubFigure at 0x7dc0ddf4a870>

Suggested improvement

Either

  • modify the docstring to clarify you might get the SubFigure (and point to how you can get the parent Figure if you want it)
    or
  • fix the method so it returns the parent Figure
@rcomer
Copy link
Member Author

rcomer commented May 4, 2024

SubFigure.get_figure does return a Figure, even if the subfigures are nested.

In [6]: sfig2 = sfig.subfigures()

In [7]: sfig2.get_figure()
Out[7]: <Figure size 640x480 with 1 Axes>

@timhoffm
Copy link
Member

timhoffm commented May 4, 2024

This seems inconsistent and likely we haven't thought about it when subfigures were introduced.

Intuitively, I'd recommend the following behavior: get_figure() returns the closest (sub)figure. If we were to go all the way up, it's not possible to get to the next level only.

That would imply:

  • Change documentation/typing of Artist.get_figure
  • Change behavior of Subfigure.get_figure to return the containing (sub)figure. - Note that get_figure only returns self.figure, so self.figure should be changed for nested subfigures. I consider it a bug that this is not the containing subfigure. @jklymak do you agree?

@jklymak
Copy link
Member

jklymak commented May 4, 2024

Hmm that seems inconsistent at least so one way or another is a bug

It seems that a subfigure wants to know its parent rather than its root?

Ooops sorry crosspost w above. I'd have to look at the code a bit.

@jklymak
Copy link
Member

jklymak commented May 5, 2024

This seems inconsistent and likely we haven't thought about it when subfigures were introduced.

Intuitively, I'd recommend the following behavior: get_figure() returns the closest (sub)figure. If we were to go all the way up, it's not possible to get to the next level only.

That would imply:

  • Change documentation/typing of Artist.get_figure
  • Change behavior of Subfigure.get_figure to return the containing (sub)figure. - Note that get_figure only returns self.figure, so self.figure should be changed for nested subfigures. I consider it a bug that this is not the containing subfigure. @jklymak do you agree?

Hmmm, I fear it's not so simple. 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.

So I wonder if the correct course of action is to override get_figure to return the parent figure, but keep self.figure as-is. In hindsight, it should have been private, I think?

@timhoffm
Copy link
Member

timhoffm commented May 5, 2024

So I wonder if the correct course of action is to override get_figure to return the parent figure, but keep self.figure as-is. In hindsight, it should have been private, I think?

It's bad enough that we have a mixture of attributes and getters, having both with slightly different semantics is even worse. I propse to

  • expand get_figure() with a parmeter to select between the direct parent and the top-level figure, choosing the appropriate default. - The default should become the same for artists and subfigures, which means an API change for one of them.
  • create whatever internal state is necessary to support the behavior (likely self._parent_figure)
  • keep self.figure with the current semantics, but discourate (deprecate?) it's use in favor of get_figure() - if needed turn it into a property.

@jklymak
Copy link
Member

jklymak commented May 5, 2024

The above seems good to me.
Something like:

get_figure(root=False)

root : bool
    If False return the (Sub)Figure this artist is on.  If True, return the root Figure if there is a nested tree of SubFigures.

@tacaswell
Copy link
Member

Ah, this also make sense as to why the subfigure_mosaic has problems with getting the nesting right.

Strongly agree we need a version that gets the parent rather than the root.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants