Skip to content

numpydoc-ify art3d docstrings #10400

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

Merged
merged 2 commits into from
Mar 16, 2018
Merged

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Feb 8, 2018

PR Summary

Convert the art3d docstrings to numpydoc / PEP-8.

Copy link
Member

@WeatherGod WeatherGod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the most part, it looks good to me. Just a couple minor points of clarification.


Returns
-------
np.array((x, y, z))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slight quibble, if a tuple was passed in, then a tuple comes back. So array-like, instead?

*zdir* Direction of text

Keyword arguments are passed onto :func:`~matplotlib.text.Text`.
'''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: If a subclass puts a docstring up in the class level (as opposed to the __init__ level), but the parent class has a docstring in its __init__, which one shows up in sphinx? I haven't checked if that is the case here, just wondering.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't checked either. Just following the numpydoc guide

The constructor (__init__) should also be documented here [class docstring], the Parameters section of the docstring details the constructors parameters.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked: Both, the class and the init docstring of the base class are replaced by the class docstring of the derived class. This is what we want in the present case, so it's ok.

----------
zsort : bool or {'average', 'min', 'max'}
For 'average', 'min', 'max' the z-order is determined by applying
the function to the z-orders of the individual polygons. *True* is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the function to the z coordinates of the vertices in the viewer's coordinate system."

Perhaps clearer, more precise. Z-order as a term is a bit loaded in matplotlib.

@timhoffm timhoffm force-pushed the art3d-docstrings branch 2 times, most recently from 467fe58 to 34bb685 Compare February 9, 2018 00:46
'''
Text object with 3D position and (in the future) direction.
'''
class Text3D(mtext.Taext):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be mtext.Text!

@tacaswell tacaswell added this to the v3.0 milestone Feb 10, 2018
Returns
-------
x, y, z : array-like
The direction vector. This is either a numpy.array *zdir* itself if
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... numpy.array or *zdir* itself ...

@WeatherGod
Copy link
Member

Other than the one little typo, this looks good to me.

@timhoffm
Copy link
Member Author

ping @WeatherGod @dstansby All requested changes are applied. Do I have to dismiss the reviews?

@timhoffm timhoffm dismissed dstansby’s stale review March 16, 2018 00:16

Requested changes applied.

@WeatherGod WeatherGod merged commit 60e682a into matplotlib:master Mar 16, 2018
@timhoffm timhoffm deleted the art3d-docstrings branch March 16, 2018 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants