improved docstrings in lib/distances.py#2070

Merged

zemanj merged 13 commits intoMDAnalysis:developfrom

zemanj:issue-2046-lib-distances-rework

Sep 12, 2018

Member

zemanj commented Sep 11, 2018

Fixes (partially) #2046, this is the third of a series of related PRs, following PRs #2048 and #2062.

Changes made in this Pull Request:

Improved documentation of all functions in MDAnalysis.lib.distances:
- Ensured that parameters referenced in docstrings correspond to function signatures.
- Properly formated references to other methods so that links are generated.
- Now uses logical instead of visual markup.
- Enforced 80 character lines.
- Standardized phrasing of repeatedly occurring parameter descriptions.
- Now uses correct type specifications for parameters and return values.
- Marked optional parameters by adding , optional after the type.
Removed all .. autofunction entries that caused duplicates in the documentation.
Improved module description to better correspond to its contents.
box parameter of capped_distance() and self_capped_distance() now also accept array_like input (now corresponding to the docs like all other functions accepting box input).

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

zemanj added 9 commits

August 21, 2018 00:44


          added missing CHANGELOG entry for PR MDAnalysis#2048

d487aa3


          ensured lib.distances.distance_array() always returns an array

6d9e3ea


          Merge branch 'develop' into issue-2046-lib-distances-rework

fd80f31


          improved docstrings in lib/_augment.pyx

bb55d16


          improved docstrings in lib/_augment.pyx

59860e7


          Merge branch 'develop' into issue-2046-lib-distances-rework

b719705


          improved docstrings in lib/_augment.pyx

280efad


          improved all docstrings in lib/distances.py

bf18cda


          Merge branch 'develop' into issue-2046-lib-distances-rework

63c0a96

zemanj changed the title ~~Issue 2046 lib distances rework~~ improved docstrings in lib/distances.py

codecov bot commented Sep 11, 2018 •

edited

Loading

Codecov Report

Merging #2070 into develop will increase coverage by 0.23%.
The diff coverage is 91.3%.

@@             Coverage Diff             @@
##           develop    #2070      +/-   ##
===========================================
+ Coverage    89.03%   89.27%   +0.23%     
===========================================
  Files          144      159      +15     
  Lines        17363    18706    +1343     
  Branches      2673     2678       +5     
===========================================
+ Hits         15460    16700    +1240     
- Misses        1300     1402     +102     
- Partials       603      604       +1

Impacted Files	Coverage Δ
package/MDAnalysis/lib/distances.py	`95.36% <91.3%> (+0.01%)`	⬆️
topology/base.py	`97.67% <0%> (ø)`
auxiliary/base.py	`89% <0%> (ø)`
util.py	`100% <0%> (ø)`
__init__.py	`91.89% <0%> (ø)`
formats/__init__.py	`100% <0%> (ø)`
auxiliary/__init__.py	`100% <0%> (ø)`
visualization/__init__.py	`100% <0%> (ø)`
coordinates/__init__.py	`100% <0%> (ø)`
dummy.py	`100% <0%> (ø)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0a2b16...4caeea2. Read the comment docs.

orbeckst approved these changes

View reviewed changes

Member

orbeckst left a comment

Thank you for taking the time to make the docs consistent and more readable.

I have a bunch of suggestions to improve the formatting and minor tweaks but I'll just approve the PR and if you want to do the changes then you can merge once you are ready.

package/MDAnalysis/lib/distances.py

               Functions
               ---------
-              .. autofunction:: distance_array(reference, configuration [, box [, result [, backend]]])

Member

orbeckst Sep 11, 2018

If it works here, i.e., just appending the definitions automatically then that's fine. If in doubt, I rather remove the :members: from the rst and list functions/classes explicitly with ..auto*: ....

Member Author

zemanj Sep 12, 2018

Removing the :members: from distances.rst and adding back the .. autofunction :: entries works, so I'll do that.

package/MDAnalysis/lib/distances.py

    
                  box : array_like, optional

                      The unitcell dimensions of the system, which can be orthogonal or

                      triclinic and must be provided in the same format as returned by

                      :attr:`MDAnalysis.coordinates.base.Timestep.dimensions`:\n

Member

orbeckst Sep 11, 2018

I didn't know that \n worked in reST.

Member Author

zemanj Sep 12, 2018

Luckily, it does! 😄

package/MDAnalysis/lib/distances.py Outdated

+                      triclinic and must be provided in the same format as returned by
+                      :attr:`MDAnalysis.coordinates.base.Timestep.dimensions`:\n
+                      ``[lx, ly, lz, alpha, beta, gamma]``.
+                  method : str, optional

Member

orbeckst Sep 12, 2018

You can provide the selection with

method : {"bruteforce", "nsgrid", "pkdtree", None}
     Keyword to override the automatic guessing ofthe employed search method.
     ``None`` chooses an optimal method.

(see https://numpydoc.readthedocs.io/en/latest/format.html#sections under 4. Parameters)

Member Author

zemanj Sep 12, 2018

The numpydoc docstring guide also says

If it is not necessary to specify a keyword argument, use optional.

And furthermore (what you referred to)

When a parameter can only assume one of a fixed set of values, those values can be listed in braces, with the default appearing first.

Listing the default None (which should come first, btw) doesn't really make sense to me, that would be like describing an optional kwarg as, for example, x : int or None.

I think a proper way of combining the two guidelines should look like this:

method : {'bruteforce', 'nsgrid', 'pkdtree'}, optional
    Keyword to override the automatic guessing of the employed search method.

The default is given in the function signature anyway.

What do you think?

package/MDAnalysis/lib/distances.py Outdated

    
                  .. SeeAlso:: :func:'MDAnalysis.lib.distances.distance_array'

                  .. SeeAlso:: :func:'MDAnalysis.lib.pkdtree.PeriodicKDTree.search'

                  .. SeeAlso:: :class:'MDAnalysis.lib.nsgrid.FastNS.search'

                  .. seealso:: :meth:`~MDAnalysis.lib.distances.distance_array`

Member

orbeckst Sep 12, 2018

This should use NUmpyDoc

See Also
--------
MDAnalysis.lib.distances.distance_array
MDAnalysis.lib.pkdtree.PeriodicKDTree.search
MDAnalysis.lib.nsgrid.FastNS.search

see https://numpydoc.readthedocs.io/en/latest/format.html#sections, 11. See Also

Member Author

zemanj Sep 12, 2018

Right, will do.

package/MDAnalysis/lib/distances.py Outdated

+                      coordinates ``reference[pairs[k, 0]]`` and
+                      ``configuration[pairs[k, 1]]``.
                   """
                   from .pkdtree import PeriodicKDTree

Member

orbeckst Sep 12, 2018

We rather have all imports at the top of the file instead of methods. If it has to be inside a method (eg to avoid circular imports) then it should be commented. See https://github.com/MDAnalysis/mdanalysis/wiki/Style-Guide#importing-modules

Member

orbeckst Sep 12, 2018

(Yes, I know, this is not your code...)

Member Author

zemanj Sep 12, 2018

I'll move the imports to the top if they don't have any side effects (which they shouldn't).

package/MDAnalysis/lib/distances.py Outdated

    
                  .. SeeAlso:: :func:'MDAnalysis.lib.distances.self_distance_array'

                  .. SeeAlso:: :func:'MDAnalysis.lib.pkdtree.PeriodicKDTree.search'

                  .. SeeAlso:: :func:'MDAnalysis.lib.nsgrid.FastNS.self_search'

                  .. seealso:: :meth:`~MDAnalysis.lib.distances.self_distance_array`

Member

orbeckst Sep 12, 2018

Use See Also numpy doc. (See above)

package/MDAnalysis/lib/distances.py Outdated

+                      triclinic and must be provided in the same format as returned by
+                      :attr:`MDAnalysis.coordinates.base.Timestep.dimensions`:\n
+                      ``[lx, ly, lz, alpha, beta, gamma]``.
+                  method : str, optional

Member

orbeckst Sep 12, 2018

document options explicitly

package/MDAnalysis/lib/distances.py

+                  -------
+                  angle : float
+                      The angle between the vectors `b`:math:`\\rightarrow`\ `a` and
+                      `b`:math:`\\rightarrow`\ `c`.

Member

orbeckst Sep 12, 2018

Maybe add a

See Also
--------
calc_angles : calculate multiple angles efficiently

Member Author

zemanj Sep 12, 2018

That is something I deliberately omitted. Since I introduced the new @check_coords() decorator, both calc_angle() and calc_angles() accept single coordinates as well as coordinate arrays, and return either an array or a single float, depending on the input.
The only difference is that calc_angle() returns the angle(s) in degrees while the result of calc_angles() is in radians. IMHO we should deprecate calc_angle().
See also my comments on calc_distance() and calc_dihedral().

package/MDAnalysis/lib/distances.py

+                  -------
+                  dihedral : float
+                      The dihedral angle between the planes spanned by the coordinates
+                      (`a`, `b`, `c`) and (`b`, `c`, `d`).

Member

orbeckst Sep 12, 2018

Maybe add a

See Also
--------
calc_dihedrals : calculate multiple dihedrals efficiently

Member Author

zemanj Sep 12, 2018

Same as above. The only difference between calc_dihedral() and calc_dihedrals() is that the former returns degrees and the latter radians. We should probably deprecate calc_dihedral().

package/MDAnalysis/lib/distances.py

+                  Returns
+                  -------
+                  distance : float
+                      The distance between positions `a` and `b`.

Member

orbeckst Sep 12, 2018

Maybe add a

See Also
--------
calc_distances : calculate multiple distances efficiently

Member Author

zemanj Sep 12, 2018 •

edited

Loading

Both functions accept both single coordinates and coordinate arrays. There is virtually no difference left between calc_distance() and calc_bonds(). We might want to deprecate calc_distance().

zemanj added 4 commits

September 12, 2018 11:46


          added back autofunction entries in MDAnalysis/lib/distances.py and re…

73ef0eb

…moved :embers: in doc/sphinx/source/documentation_pages/lib/distances.rst instead


          use numpydoc See Also sections instead of .. seealso::

791ecd9


          numpydoc selections for parameters with fixed value sets

c9567dc


          moved nsgrid import to top of lib/distances.py

4caeea2

zemanj merged commit 6741d1a into MDAnalysis:develop

zemanj mentioned this pull request

MDAnalysis.lib.distances needs rework #2046

Open

51 tasks

Member

richardjgowers commented Sep 12, 2018

@zemanj if you want to deprecate calc_angle etc you can just remove them completely, they've never been released. It might be cleaner to have fewer functions

Member Author

zemanj commented Sep 12, 2018

@richardjgowers I opened an issue (#2072) regarding the obsolete functions. Thanks for the info!
@orbeckst Thank you for the quick and very constructive review! 👍

zemanj mentioned this pull request

bug fixes in lib.distances #2083

Merged

4 tasks

zemanj mentioned this pull request

Issue 2046 lib distances rework continued #2114

Merged

4 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet