Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
DOC: organize figure API#26629
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
DOC: organize figure API#26629
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
doc/api/figure_api.rst Outdated
Figure.draw_without_rendering | ||
Figure.draw_artist | ||
Other |
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.
can this go in annotate since this is also about labeling?
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 such a bizarre API I hesitate to add it at all. This should be part of the Axes API and has no business on Figure.
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.
maybe so, but I think putting it in it;s very own section inadvertently gives it more attention
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.
OK, done - not a huge fan, but its innocuous enough.
doc/api/figure_api.rst Outdated
The Figure and SubFigure classes | ||
================================ | ||
Every visualization starts with a `~.Figure` class, often instantiated via `pyplot.figure`, `.pyplot.subplots`, or `.pyplot.subplot_mosaic`. It is possible to have `~.SubFigure` nested inside a `~.Figure`, so many common methods are defined in `~.FigureBase`. |
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.
Every visualization starts with a `~.Figure` class, often instantiated via `pyplot.figure`, `.pyplot.subplots`, or `.pyplot.subplot_mosaic`. It is possible to have `~.SubFigure` nested inside a `~.Figure`, so many common methods are defined in `~.FigureBase`. | |
Every visualization starts with a `~.Figure` class, often instantiated via `pyplot.figure`, `.pyplot.subplots`, or `.pyplot.subplot_mosaic`. It is possible to have `~.SubFigure`objects nested inside a `~.Figure`, so many common methods are defined in `~.FigureBase`. |
I think this maybe needs a sentence on what is the difference? I'm seeing the (sub) throughout here and am very confused on which when why
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.
Changed to:
Every visualization starts with a `~.Figure` class, often instantiated via `pyplot.figure`, `.pyplot.subplots`, or `.pyplot.subplot_mosaic`. Matplotlib has the concept of a `~.SubFigure`, which is a logical figure inside a parent `~.Figure`. Most common methods are defined in `~.FigureBase`, but there are a few methods that only make sense on the parent `~.Figure`.
let me know if that is any clearer
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.
Mostly clearer thanks, though I think instead of logical figure maybe subclass might be clearer.
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 now under Subfigure
SubFigure=========Matplotlib has the concept of a `~.SubFigure`, which is a logical figure insidea parent `~.Figure`. It has many of the same methods as the parent. See:ref:`nested_axes_layouts`.
doc/api/figure_api.rst Outdated
fig = plt.figure(layout='constrained', figsize=(4, 2.5), facecolor='lightgoldenrodyellow') | ||
# Make two subfigures, left ones more narrow than right ones: | ||
sfigs = fig.subfigures(1, 2, width_ratios=[0.8, 1]) | ||
sfigs[0].set_facecolor('khaki') | ||
sfigs[1].set_facecolor('lightsalmon') | ||
# Add subplots to left subfigure: | ||
lax = sfigs[0].subplots(2, 1) | ||
sfigs[0].suptitle('Left subfigure') | ||
# Add subplots to right subfigure: | ||
rax = sfigs[1].subplots(1, 2) | ||
sfigs[1].suptitle('Right subfigure') | ||
# suptitle for the main figure: | ||
fig.suptitle('Figure') | ||
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.
Probably sick of hearing this from me, but this is my usual concern that this is doing both too much and not enough - what I mean is that it feels like it's trying to show off some of the capabilities of the figure creation (like the width kwargs) but it's also not comprehensive. If there must be code (and I don't think there should be) then I think it should be at a consistent level: here is how we create figures, here is how we create subfigures - here is us showing how to use one method on both objects, and then "to learn more about this, see user guide
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 code is not shown by default, though of course the user can download it if curious. The goal is to visually orient the reader as to what a Figure is versus a SubFigure.
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.
I like visuals, but feel this is disproportional here. While SubFigure is an advanced concept that is maybe relevant for 10% of our users, the plot draws more attention to SubFigure than to Figure.
It's good that the code is not shown. I'd even claim it's not needed at all: People don't need to know how to create that figure exactly. To learn how to do SubFigures, they should go to the linked docs.
I suggest to reduce this even further to a conceptual graphic, like we're doing for the Axes creation in#26417:
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.
While SubFigure is an advanced concept that is maybe relevant for 10% of our users, the plot draws more attention to SubFigure than to Figure.
Fair enough - the cross linking issues were such that this had to get moved down to the bottom of this page anyways.
It's good that the code is not shown. I'd even claim it's not needed at al
I'm not a fan of using graphviz or creating standalone svg's to document Matplotlib's graphics features...
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 visual is now under Subfigure.
Obviously we disagree on whether such signposts are helpful. I suppose someone else will have to decide.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
doc/api/figure_api.rst Outdated
Figure.axes | ||
Figure.delaxes | ||
Saving a Figure |
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.
Saving a Figure | |
Save Figure |
consistent voice in titles?
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.
Fixed all the titles - removed redundant "Figure" from them...
Uh oh!
There was an error while loading.Please reload this page.
8d356c2
to7b579a6
CompareThere 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.
There's the downside with the Figure link targets, but I think this should not stop the PR. The usability gain is substantial.
I'll wait with merging in case somebody wants to comment on that.
The Figure class | ||
================ | ||
.. autosummary:: | ||
:toctree: _as_gen | ||
:template: autosummary_class_only.rst | ||
:nosignatures: | ||
Figure |
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.
A downside of this - and I just realized that we already have the issue forAxes
- that all code links`.Figure`
go to thesub-page, whereas logically it would make more sense to have them point to the index page.
I have no idea how to improve on this with existing means (well, one could introduce a custom directive:c:`Figure`
, but (i) I'm reluctant to give up on the simpler formatting of`.Figure`
and (ii) since that is a non-standard appoach, it's likely that new standard refrences`.Figure`
will keep creeping in.) I've opened anissue to sphinx. Maybe we have to live with that for now.
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.
I see what you are saying, but I'm not convinced it is a huge problem:
`matplotlib.figure`
is the right way to get the overall index if that is what we want in the docs.
`.Figure`
takes us to the instantiation docs, which I think we donot want in the overview.
At least on non-mobile, the left-hand navigationand the breadcrumb is pretty clear where the module level description is:

An alternative may be that we make entries for matplotlib.figure.Figure and matplotlib.figure.SubFigure. We don't do this with any other APIs currently, but I don't think there is any reason they API reference has to slavishly correspond to files in the library directory. I think this would meanfigure_Figure_api.rst
andfigure_SubFigure_api.rst
, but that doesn't seem the end of the world.
Finally, an even more radical suggestions would be to rearrange the source code itself; eg have a subfigure module, and splitFigureBase
into its own private module. That doesn't seem the end of the world - I doubt many people are doingimport matplotlib.figure.SubFigure
yet.
So, I think the proposed change here is better than what we currently have, but I'm open to relatively major surgery if we can get something even better.
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.
Thinking about this more though: even if we do the more radical suggestions above, I don't see how we get the init docs onto their own page and out of the intro.
Your suggestion to sphinx seems reasonable. Not sure how we emulate that internally.
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.
One could try to separate class and init docstring, put the class on the overview page and init in subpage where the class currently is. May be possible through appropriate configurationhttps://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#configuration.
But that can be a follow up PR. I suggest to merge this as is for now.
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.
I think a fair bit is possible with templates as well.
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.
See newest version...
jklymak commentedSep 1, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
So this version the init docstring shows up on the main api page, but I think Compare here:https://output.circle-artifacts.com/output/job/66aaf31d-2c9c-478b-80dd-d816d8157d4f/artifacts/0/doc/build/html/api/figure_api.html# |
👍 for the TOC.
IMHO this doesn't really cut it:
I'd rather stick with the previous solution, merge, and fiddle with improvements in a subsequent PR. Note thatAxes has the same issue, and it's an argument to treat them synchronously. That said, my approval stands with either version. They are both an improvement, should be merged soon, and could need further improvement IMHO. Ping@story645 is your change request reasonably addressed? |
I'm fine putting it back the way it was. Do we need the TOC though if we don't have anything interrupting access to the table? |
Either way is fine for me. |
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.
doc/api/figure_api.rst Outdated
Figure.get_constrained_layout_pads | ||
Interacting |
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.
Interacting | |
Interactivity |
slight nit that I think this tends to be common usage so folks are more likely to parse quickly
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.
Except then the voice would be inconsistent.
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.
hmm - Interactivity acts like a noun like layout rather than a verb like annotating, so it's still consistent?
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.
OK, changed to "Interactive", which is what's used for all the other API pages (for better or worse). We can search and replace to "Interactivity" if that is better as a follow-up, but at least now it's consistent.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
.. plot:: | ||
fig = plt.figure(layout='constrained', figsize=(4, 2.5), facecolor='lightgoldenrodyellow') |
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 probably the silliest little nit but I wish this figure was a bit wider to match the text width.
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.
@story645 you are blocking on this. |
doc/api/figure_api.rst Outdated
Figure.get_constrained_layout_pads | ||
Interacting |
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.
hmm - Interactivity acts like a noun like layout rather than a verb like annotating, so it's still consistent?
Since this has changes in .py files, we cannot backport to 3.8-doc. |
This was accidentally lost throughmatplotlib#26629.
Uh oh!
There was an error while loading.Please reload this page.
PR summary
This organizes the Figure API in a similar way to the Axes API.
Figure
is a top-level API, so deserves proper curation.Updated a couple of
figure.py
docstrings to have better slugs.Edit: built version:https://output.circle-artifacts.com/output/job/a4d69671-bb03-4c9a-a6cc-863209e4ac71/artifacts/0/doc/build/html/api/figure_api.html
PR checklist