[MRG+1] [DOC] Turn ginput dostring into a numpydocstring #8172

dstansby · 2017-02-28T15:27:22Z

No description provided.

afvincent · 2017-02-28T16:18:02Z

lib/matplotlib/figure.py

+        ----------
+        n : int, optional
+            Number of mouse clicks to accumulate. If negative, accumulate
+            clicks until the input is terminated manually.


Add "Default is 1."?

anntzer · 2017-02-28T23:11:33Z

lib/matplotlib/figure.py

-        (or potentially both mouse buttons at once) terminates the input.
-
-        Right clicking cancels last input.
+        Blocking call to interact with a figure. This will wait for *n*


I would rewrite the paragraph as

Wait until the user clicks n times on the figure. Returns the coordinates of each click in a list.

Also, I believe (though it is very unsure) that we decided to drop the "*", which are a remainer of the past in Matplotlib's documentation.

Not for arguments, see http://matplotlib.org/devdocs/devel/documenting_mpl.html#formatting.

NelleV

I think this is a great improvement :)

NelleV · 2017-03-02T03:08:34Z

lib/matplotlib/figure.py

+        ----------
+        n : int, optional
+            Number of mouse clicks to accumulate. If negative, accumulate
+            clicks until the input is terminated manually. Default is 1.


the numpydoc formats puts the default on the first line:

n : int, optional, default: 1

I find the numpydoc "offical" version more readable.

NelleV · 2017-03-02T03:09:22Z

lib/matplotlib/figure.py

-        (or potentially both mouse buttons at once) terminates the input.
-
-        Right clicking cancels last input.
+        Blocking call to interact with a figure. This will wait for *n*


Also, I believe (though it is very unsure) that we decided to drop the "*", which are a remainer of the past in Matplotlib's documentation.

anntzer · 2017-03-02T17:55:53Z

lib/matplotlib/figure.py

+        Returns
+        -------
+        points : list of tuples
+            The clicked points. ``len(points)`` will equal the number of points


I would rewrite the entire thing (the three sentences) as A list of (x, y) coordinates of the clicks. There is no "number of requested points" if n<0, and there can be fewer points if the user presses mouse_stop prematurely.

Yep, sounds better.

QuLogic

LGTM modulo one item.

QuLogic · 2017-03-02T21:46:07Z

lib/matplotlib/figure.py

-        (or potentially both mouse buttons at once) terminates the input.
-
-        Right clicking cancels last input.
+        Blocking call to interact with a figure. Wait until the user clicks *n*


The first line should be by itself with the remaining description on another paragraph (one blank line later).

anntzer · 2017-03-02T22:20:43Z

lib/matplotlib/figure.py

+            Number of seconds to wait before timing out. If zero or negative
+            will never timeout.
+        show_clicks : bool, optional, default: False
+            If True, show a red cross at the location of each click


Add final dots here and below (including returns), and squash? (otherwise that's a lot of commits :-))

In addition to the comment above, I think the format should be, for example

show_clicks : bool, optional (default = False)

That format seems much rarer in our codebase than the existing format.

@QuLogic I will approve the PR after this (the dots -- squashing is up to you) is fixed.

The format for defaults should probably added to the documenting matplotlib developer's guide. Note that numpy itself gives the example

Description of parameter `x` (the default is -1, which implies summation over all axes).

(which is different and I dislike)

I think you meant to tag @dstansby there; I'm fine with this when you are.

anntzer · 2017-03-05T23:33:52Z

lib/matplotlib/figure.py

+            Number of seconds to wait before timing out. If zero or negative
+            will never timeout.
+        show_clicks : bool, optional, default: False
+            If True, show a red cross at the location of each click


@QuLogic I will approve the PR after this (the dots -- squashing is up to you) is fixed.

Small fixes Add default value for npoints Suggested changes to ginput docstring Better description of ginput return Put summary line by itself at the top Add in missing full stops

dstansby · 2017-03-07T13:27:24Z

I've added in the extra full stops, and given it a squash.

NelleV · 2017-03-08T05:03:11Z

Thanks @dstansby !

dstansby added the Documentation label Feb 28, 2017

afvincent reviewed Feb 28, 2017

View reviewed changes

anntzer reviewed Feb 28, 2017

View reviewed changes

NelleV reviewed Mar 2, 2017

View reviewed changes

anntzer reviewed Mar 2, 2017

View reviewed changes

QuLogic reviewed Mar 2, 2017

View reviewed changes

anntzer reviewed Mar 2, 2017

View reviewed changes

anntzer requested changes Mar 5, 2017

View reviewed changes

anntzer mentioned this pull request Mar 6, 2017

FIX: eventplot 'colors' kwarg (#8193) #8204

Closed

dstansby changed the title ~~Turn ginput dostring into a numpydocstring~~ [DOC] Turn ginput dostring into a numpydocstring Mar 7, 2017

Make ginput dostring into a numpydocstring

0151c05

Small fixes Add default value for npoints Suggested changes to ginput docstring Better description of ginput return Put summary line by itself at the top Add in missing full stops

dstansby force-pushed the ginput-docstring branch from 8d53729 to 0151c05 Compare March 7, 2017 13:26

anntzer approved these changes Mar 7, 2017

View reviewed changes

anntzer changed the title ~~[DOC] Turn ginput dostring into a numpydocstring~~ [MRG+1] [DOC] Turn ginput dostring into a numpydocstring Mar 7, 2017

NelleV merged commit 8277ddf into matplotlib:master Mar 8, 2017

dstansby deleted the ginput-docstring branch April 2, 2017 16:44

QuLogic added this to the v2.1 milestone Aug 12, 2021

Uh oh!

[MRG+1] [DOC] Turn ginput dostring into a numpydocstring #8172

[MRG+1] [DOC] Turn ginput dostring into a numpydocstring #8172

Uh oh!

Conversation

dstansby commented Feb 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NelleV left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dstansby commented Mar 7, 2017

Uh oh!

NelleV commented Mar 8, 2017

Uh oh!

Uh oh!