Replace ACCEPTS by standard numpydoc params table. #11397

anntzer · 2018-06-07T17:17:50Z

(In the cases where this is "easy".)

Followup to #11300.

PR Summary

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

timhoffm · 2018-06-07T17:58:57Z

lib/matplotlib/axis.py

-        ACCEPTS: float
+        Parameters
+        ----------
+        val : float


Would it make sense to use type annotations for these very simple cases? The type could be inferred from the type annotation. We could leave out the Parameters section which is quite verbose just for specifying a simple type, both in source code and in the generated html.

That's a discussion for another time...

Well, just saying. It's just one line in the ArtistInspector (actually none, because you can delete another line for that). And it saves a lot of boilerplate docstring code.

I'd be more comfortable if we can draw a policy as to where to draw the line between strict annotations and "vague" type docstrings. Especially for Matplotlib, there are many cases where the exact type is complicated or it's actually the value (rather than just the type) that's important.

The policy for the docstring is: Describe the expected input parameters in a way, that is easy to read and understand by a human user. The numpydoc howto gives some helpful guidelines that should be followed if possibe.

I currently would only use type annotations for simple cases as the one above. Anyway, should we have both later on, the docstring type should be given preference. It's really only that you don't need verbose boilerplate code like

Parameters ------------ theparam : float

But I don't hold you up on this.

(In the cases where this is "easy".)

jklymak

This really cleans things up a lot! Thanks for the huge effort. Some questions, but some of them could be future PRs (particularly the issues w/ 'color')...

jklymak · 2018-06-14T16:26:06Z

lib/matplotlib/axes/_base.py

        Parameters
        ----------
-        value : {"linear", "log", "symlog", "logit"}
+        value : {"linear", "log", "symlog", "logit", ...}


I know there is an ellipsis above, but what does it mean?

It's all the scales registered in the _scale_mapping dict.

jklymak · 2018-06-14T16:26:55Z

lib/matplotlib/axes/_base.py

        Parameters
        ----------
-        anchor : str or 2-tuple of floats
+        anchor : 2-tuple of floats or {'C', 'SW', 'S', 'SE', ...}


Is the ellipsis because its obvious what the other arguments are? Maybe not so obvious for non-english readers?

It’s explained in the following docstring.

jklymak · 2018-06-14T16:27:10Z

lib/matplotlib/axes/_base.py

        Parameters
        ----------
-        value : {"linear", "log", "symlog", "logit"}
+        value : {"linear", "log", "symlog", "logit", ...}


jklymak · 2018-06-14T16:28:27Z

lib/matplotlib/axis.py

-        ACCEPTS: A :class:`~matplotlib.ticker.Formatter` instance
+        Parameters
+        ----------
+        formatter : ~matplotlib.ticker.Formatter


Sorry for my ignorance and laziness to not check the render - does this resolve to a link?

Apparently it doesn't (https://3226-7439715-gh.circle-artifacts.com/0/home/circleci/project/doc/build/html/api/_as_gen/matplotlib.axis.Axis.set_major_formatter.html#matplotlib-axis-axis-set-major-formatter), but some minimal testing (on another project) shows that napoleon (instead of numpydoc) does resolve it. Which, I guess, is a good reason to revive #5743 (unless I missed some way to achieve this with numpydoc).

I'll ask one of the numpydoc devs. In the meantime, let's move forward on that without this sorted out. This is a great improvement on what we had before.

jklymak · 2018-06-14T16:29:41Z

lib/matplotlib/collections.py

+
+        Parameters
+        ----------
+        c : matplotlib color arg or sequence of rgba tuples


Is there a way to link the color arg page here?

Probably, but I'd rather you just open an issue and we try to resolve that as a separate thing.

jklymak · 2018-06-14T16:31:54Z

lib/matplotlib/lines.py

        Parameters
        ----------
-        every: None | int | length-2 tuple of int | slice | list/array of int \
-| float | length-2 tuple of float
+        every: None or int or (int, int) or slice or List[int] or float or \


Do we need the 'or' after everything in this list for some reason? one of: None, int, (int, int), slice, List[ind], float, or (float, float) would be easier to read. Or separate by semicolons?

See http://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#info-field-lists ("Multiple types in a type field will be linked automatically if separated by the word “or”").

jklymak · 2018-06-14T16:33:50Z

lib/matplotlib/patches.py

-        ACCEPTS: mpl color spec, None, 'none', or 'auto'
+        Parameters
+        ----------
+        color : color or None or 'auto'


We have a :rc: alias, right? Do we need a :color: alias so that all instances of color can point to the definition?

Probably a good separate issue, per above.

jklymak

This is big enough that maybe someone else should approve as well. @timhoffm are you OK w/ this now?

timhoffm

Basically ok, even though I'm still not a fan of adding verbose boilerplate Parameter sections just to define a simple type for a parameter.

The type definitions should use full names and not be abbreviated by ~.

NelleV · 2018-06-22T22:45:29Z

I don't think any of the comments are blockers, so I'm going to go ahead and merge it. Thanks everyone!

timhoffm reviewed Jun 7, 2018

View reviewed changes

anntzer force-pushed the noACCEPTS branch from e7a2437 to dcafe5d Compare June 7, 2018 18:14

Replace ACCEPTS by standard numpydoc params table.

ff4ac6d

(In the cases where this is "easy".)

anntzer force-pushed the noACCEPTS branch from dcafe5d to ff4ac6d Compare June 7, 2018 21:27

jklymak reviewed Jun 14, 2018

View reviewed changes

anntzer mentioned this pull request Jun 15, 2018

DOC: Use napoleon sphinx extension #5743

Closed

jklymak approved these changes Jun 19, 2018

View reviewed changes

jklymak added the Documentation label Jun 19, 2018

jklymak added this to the v3.0 milestone Jun 19, 2018

timhoffm reviewed Jun 19, 2018

View reviewed changes

NelleV approved these changes Jun 22, 2018

View reviewed changes

NelleV merged commit e09dece into matplotlib:master Jun 22, 2018

anntzer deleted the noACCEPTS branch July 9, 2018 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace ACCEPTS by standard numpydoc params table. #11397

Replace ACCEPTS by standard numpydoc params table. #11397

anntzer commented Jun 7, 2018

timhoffm Jun 7, 2018

anntzer Jun 7, 2018

timhoffm Jun 7, 2018

anntzer Jun 7, 2018

timhoffm Jun 7, 2018 •

edited

Loading

jklymak left a comment

jklymak Jun 14, 2018

anntzer Jun 15, 2018

jklymak Jun 14, 2018

timhoffm Jun 14, 2018

jklymak Jun 14, 2018

jklymak Jun 14, 2018

anntzer Jun 15, 2018

NelleV Jun 22, 2018

jklymak Jun 14, 2018

anntzer Jun 15, 2018

jklymak Jun 14, 2018

anntzer Jun 15, 2018

jklymak Jun 14, 2018

anntzer Jun 15, 2018

jklymak left a comment

timhoffm left a comment

NelleV commented Jun 22, 2018

Replace ACCEPTS by standard numpydoc params table. #11397

Replace ACCEPTS by standard numpydoc params table. #11397

Conversation

anntzer commented Jun 7, 2018

PR Summary

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

jklymak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak left a comment

Choose a reason for hiding this comment

timhoffm left a comment

Choose a reason for hiding this comment

NelleV commented Jun 22, 2018

timhoffm Jun 7, 2018 •

edited

Loading