DOC improved subplots' docstring #7232
Conversation
|
ping @stefanv @michaelpacer |
| >>> ax1.set_title('Sharing Y axis') | ||
| >>> ax2.scatter(x, y) | ||
|
|
||
| Creates four polar axes, and access them throught the returned array |
There was a problem hiding this comment.
FYI, that's the lack of coffee…
| >>> ax1.set_title('Sharing Y axis') | ||
| >>> ax2.scatter(x, y) | ||
|
|
||
| Creates four polar axes, and access them throught the returned array |
| - If True, extra dimensions are squeezed out from the returned axis | ||
| object: | ||
| - if only one subplot is constructed (nrows=ncols=1), the | ||
| resulting single Axis object is returned as a scalar. |
There was a problem hiding this comment.
This does not look indented correctly, and appears to render wrong (and same for the following two.)
| Returns: | ||
|
|
||
| Returns | ||
| ------- | ||
| fig, ax : tuple |
There was a problem hiding this comment.
These can be split into separate entries, right?
|
There is still an indentation problem, but I am not seeing it… |
|
When I look at the docstring, it looks like exactly what Stefan and I were talking about (and what I'd been looking for). But the rendered image included in the PR seems to cut off before the new example is displayed. Fortunately, that image doesn't matter while the docstring does. Thanks @NelleV! |
| :meth:`~matplotlib.figure.Figure.add_subplot` call used to | ||
| create each subplots. | ||
| :meth:`~matplotlib.figure.Figure.add_subplot` call used to create each | ||
| subplots. |
There was a problem hiding this comment.
maybe "create each subplot"?
| >>> ax.plot(x, y) | ||
| >>> ax.set_title('Simple plot') | ||
|
|
||
| Creates two subplots and unpack the output array immediately |
| ncols > 1 then the y tick labels won't be displayed on any of | ||
| the plots but the ones on the first column. | ||
|
|
||
| squeeze : bool, optional, default: True |
There was a problem hiding this comment.
I think there is an indentation problem around here, but I can't see it.
There was a problem hiding this comment.
I found the problem and fixed it.
- the dostring is now in numpydoc format. - added an extra line in the examples to showcase accessing the axes returned array closes #7230
|
|
||
| fig, ax : tuple | ||
| ax : axis or array of axis objects. |
There was a problem hiding this comment.
They're really Axes objects, or more specifically AxesSubplot objects. An Axis object is the individual x- or y-axis.
|
|
||
| Examples:: | ||
| Creates just a figure and only one subplot. |
There was a problem hiding this comment.
I would go with imperative tense ("Create just ..."); the sharing sections use this tense as well. Also, this is the only one with a period at the end.
|
|
||
| # Four polar axes | ||
| plt.subplots(2, 2, subplot_kw=dict(polar=True)) | ||
| Share a X and Y axis with all subplots |
| - "all" has the same effect as True. | ||
| - "none" has the same effect as False. | ||
| - If "row", each subplot row will share a X axis. | ||
| - If "col", each subplot column will share a X axis. Note that if |
There was a problem hiding this comment.
This comment (which is also present for "if True...") could possibly be factored out to the end of the list: "if the X axis is shared across columns (sharex=True or sharex="col"), then etc." (and similarly for sharey).
|
|
||
| Note that if the x-axis is shared across rows (sharex=True or | ||
| sharex="col") and nrows > 1 then the x tick labels won't be displayed | ||
| on any of plots but the ones on the bottom row. |
There was a problem hiding this comment.
Upon further reflection, I think you can state this more simply as "Note that if the x-axis is shared across rows (...), then the x tick labels will only be displayed on the bottom row" (skipping the remark about nrows > 1).
(By the way let's be consistent between "x-axis" and "X axis" (I prefer the former)).
|
Can you update the figure at the top? The AppVeyor fail appears unrelated. |
|
I am not sure what figure you are referring to. |
|
Of the rendered documentation in the first post (not in the docs themselves.) |
|
Here it is (it's tiny as I had to zoom out a lot to be able to take a reasonable size screenshot)
|
|
Sorry to be late in commenting on this, but I am wondering whether you could condense the docstring by treating nrows and ncols together, and similarly with sharex and sharey. There is value in making docstrings as concise as possible without losing important info. |
|
|
||
| sharex : bool or string, optional, default: False | ||
| - If True, the x-axis will be shared amongst all subplots. | ||
| - If False, no axis will be shared amongst subplots. |
|
@efiring nrows and ncols, certaintly. The sharex sharey condensed is actually very hard to read (and to write) |
|
Maybe something like the following: |
|
I tried to just push a commit with my pedandtic word changes, but it would not let me. index de97b5d..2205698 100644
--- a/lib/matplotlib/figure.py
+++ b/lib/matplotlib/figure.py
@@ -1056,7 +1056,7 @@ class Figure(Artist):
squeeze : bool, default: True
- If True, extra dimensions are squeezed out from the returned
- axis object:
+ object:
- if only one subplot is constructed (nrows=ncols=1), the
resulting single Axes object is returned as a scalar.
diff --git a/lib/matplotlib/pyplot.py b/lib/matplotlib/pyplot.py
index d304876..e112a27 100644
--- a/lib/matplotlib/pyplot.py
+++ b/lib/matplotlib/pyplot.py
@@ -1077,7 +1077,7 @@ def subplots(nrows=1, ncols=1, sharex=False, sharey=False, squeeze=True,
subplots of the first column.
squeeze : bool, optional, default: True
- - If True, extra dimensions are squeezed out from the returned Axes
+ - If True, extra dimensions are squeezed out from the returned
object:
- if only one subplot is constructed (nrows=ncols=1), the
|
|
@tacaswell that's weird: the tick box to allow edits from maintainers is ticked… |
|
Before I do yet another commit, can you be more specific about the word change you'd like? The review and patch aren't the same. |
|
@NelleV, and other reviewers, please reconsider your rejection of my attempt to make a much shorter docstring. I believe we need to think again about what makes a good docstring. Consider what you want to see, either in the terminal or in the browser: a concise description, with the most critical information appearing early. It should not insult the intelligence of the viewer, so it should minimize unnecessary repetition. Some standardization of structure, such as that imposed by numpydoc, is OK, but the text should look like it was written by a person, not an algorithm. With those ideas in mind, I think that we should use lists sparingly, with minimal nesting, because they gobble space. And I don't see that a combined description of sharex and sharey is necessarily any harder or slower to read and digest than the present separate description. Surely my draft suggestion can be improved, but I think it illustrates a better path. |
|
I am inclined to agree with @efiring about the fact that a too long or a too repetitive docstring can be hard to read or navigate, and that we should value docstring conciseness when it does not harm the docstring readibility nor the informations it provides. Nonetheless I still find first level lists to be useful to provide fast and random access to descriptive information. In the present case, as the description of It is 3 lines longer than the super squeezed version, but only half the length of the non-factorized one. To be honest, I struggled a bit to fit each item of the list into a single (PEP8) line: I hope English is still correct. |
|
@afvincent, thank you, that looks good to me. Recommended tweak: Each leading "If" could be removed, and the first comma replaced by a colon, e.g., In the second item (False), "all" -> "each". |
|
I agree that shorter docstrings are in general better. Where I am more concerned is the amount of comments and debates on a pull request that original meant to add an example to the docstring. With half of the effort on that pull request, we probably could have done 5 more improvements on other docstrings. |
|
On 2016/10/12 6:55 AM, Nelle Varoquaux wrote:
Good point. This is a real problem. I think that what I should have |
| - True or 'all': x- or y-axis will be shared among all | ||
| subplots. | ||
| - False or 'none': all subplot x- or y-axis will be | ||
| independant. |
There was a problem hiding this comment.
| - True or 'all': x- or y-axis will be shared among all | ||
| subplots. | ||
| - False or 'none': all subplot x- or y-axis will be | ||
| independant. |
There was a problem hiding this comment.
Same minor remark: "independant" ← "independent", and "all" ← "each".
That's not all it did though, and sometimes things need to be discussed before they get to a good consensus. It's easier to do so on one PR than a whole bunch of them, especially if they're all working on the same code/docs. |
|
@QuLogic First, these discussions should not happen on PRs, as it doesn't include the whole community. Second, that's not the first PR were this happens: most of the documentation PRs I have seen take a very long time to be merged. It is much easier to nitpick and bikeshed on a docstring than on tiny code patch that fixes a bug hard to track (#7214 has been opened for 9 days, yet only has one approval and potentially three people looking at it). You should check Pieter Hintjens' view on the question: http://hintjens.com/blog:106 |
| sharex, sharey : bool or {'none', 'all', 'row', 'col'}, default: False | ||
| Controls sharing of properties among x (`sharex`) or y (`sharey`) | ||
| axes: | ||
| - True or 'all': x- or y-axis will be shared among all |
There was a problem hiding this comment.
I think you need a blank line here because sphinx
There was a problem hiding this comment.
yep… I fixed that (but haven't tested) and took this opportunity to rebase.
|
I think that the threshold on documentation only PRs should be "Does this make it better and is not wrong?", if yes then merge. Documentation seems to be much harder to collaborate on via PR comments than code. I suggest that the next time we see one of these PRs getting very bike sheddy, the (3 most) interested parties arrange for some sort of real-time collaboration (google docs / hackpad / whatever) + voice and hash it out. If sphinx passes, I intend to merge this. |
DOC: improved subplots' docstring
Conflicts:
lib/matplotlib/figure.py
- figure.py subplots method does not exist on 2.x branch
|
Thanks everyone! backported to v2.x as 2c7be7c |
closes #7230
Here is how it now looks rendered: