-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
plt.subplots and plt.figure docstring changes #11382
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
Conversation
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.
Thanks! Looks good overall. I'd like still some minor changes.
lib/matplotlib/pyplot.py
Outdated
|
||
frameon : bool, optional, default: True | ||
If False, suppress drawing the figure frame. | ||
|
||
FigureClass : class derived from matplotlib.figure.Figure | ||
FigureClass : class derived from the :class:`~matplotlib.figure.Figure` class. |
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.
FigureClass : subclass of `~matplotlib.figure.Figure`
should be sufficient.
lib/matplotlib/pyplot.py
Outdated
Optionally use a custom Figure instance. | ||
|
||
clear : bool, optional, default: False | ||
If True and the figure already exists, then it is cleared. | ||
|
||
Returns | ||
------- | ||
figure : Figure | ||
figure : :class:`~matplotlib.figure.Figure` object |
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.
figure : `~matplotlib.figure.Figure`
lib/matplotlib/pyplot.py
Outdated
pylab interface. Additional kwargs will be passed to the figure init | ||
function. | ||
pylab interface. Additional kwargs will be passed to the | ||
:class:`~matplotlib.figure.Figure` init function. |
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.
We use the sphinx default role 'obj' so this can be simplified. No need for the Prefix. Within text blocks, we use the shortest form possible.
`~matplotlib.figure.Figure` or maybe even `.Figure` is enough.
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 have mostly looked at the other docstrings in regard to prefixes. Are there any directives for how to use them (couldn't find anything in the documentation)? :func: and :meth: adds parenthesis but are there any other reasons to use them?
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.
Unfortunately, the docs on documentation is not up-to-date in that respect. See Citing other types in #10225.
That using the default role does not add parentheses is a bug in Sphinx: sphinx-doc/sphinx#4409 I wouldn't make that a criterion how to write our docstrings.
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 wanted to know if we should use for example
:class: .Figure
or just .Figure
Should :class:, :meth:, :func:, :mod: etc be used or not? I haven't found anything about this anywhere but some docstrings include those but I believe that many don't.
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.
Citing other types in #10225 is the current proposal.
Don't rely too much on what's currently present in the docs. We have a lot of legacy code that has not yet been updated to and still uses :class:`.Figure`
everywhere.
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, so :class: etc should not be used? I found that section confusing when it seems to only talk about when to use ~matplotlib.markers.MarkerStyle or .MarkerStyle and not anything about :class: etc.
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.
Oh sorry, I didn't read carefully.
:class: is basically almost never needed (except for rare cases where name may not be unique otherwise, e.g. cycler
).
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 updated the code.
I still don't understand what is the best practice, convention and what works in regard to :func:, :class: etc.
I need to use :func: and :meth: in places where I don't understand why. It seems to not look outside of the actual function or method without them when searching for the function or method even though I even used the full link.
lib/matplotlib/pyplot.py
Outdated
width, height in inches. If not provided, defaults to rc | ||
figure.figsize. | ||
width, height in inches. If not provided, defaults to | ||
:rc:`figure.figsize`. |
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.
It was suggested in another thread, that we add the default rcParams value in brackets, such as
defaults to :rc:`figure.figsize` (``[6.4, 4.8]``)
I think that's a reasonable addition. While :rc:
could be extended to write that as well, there's a benefit in having the value within the docstring itself.
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 dont like the look in the rendered html
rcParams["figure.figsize"] ([6.4, 4.8])
rcParams["figure.figsize"], ([6.4, 4.8])
Should one of those be used or are there any better way?
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 was just suggested in another thread. We do not have a convention yet and I have no clear idea how this formatting should be. Maybe rcParams["figure.figsize"] = [6.4, 4.8]
?. Do you have any suggestions?
lib/matplotlib/pyplot.py
Outdated
|
||
ax : Axes object or array of Axes objects. | ||
|
||
ax can be either a single :class:`matplotlib.axes.Axes` object or an | ||
ax can be either a single :class:`~matplotlib.axes.Axes` object or an |
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.
*ax*
ac8c3d1
to
31f709a
Compare
lib/matplotlib/figure.py
Outdated
ax : Axes object or array of Axes objects. | ||
*ax* can be either a single `~matplotlib.axes.Axes` object or | ||
an array of Axes objects if more than one subplot was created. The | ||
dimensions of the resulting array can be controlled with the squeeze |
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.
PEP-8 claims the line is too long: break before "squeeze" That's what breaks the Travis build.
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.
Good to go modulo the PEP-8 error.
Thanks @fredrik-1 ! |
Same small changes in
plt.subplots
andplt.figure
docstrings and theplt.subplots
docstring was copied to thefigure.subplots
docstring.The changes was mainly in the links to other documentation
PR Checklist