fix sphinx warnings #1312

kain88-de · 2017-04-16T15:13:20Z

Changes made in this Pull Request:

This only fixes the warnings when we search for modules. The cross-reference
warnings aren't fixed. But that seems to be a bug inside of sphinx.

PR Checklist

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

kain88-de · 2017-04-16T15:13:50Z

package/doc/sphinx/source/documentation_pages/lib/c_distances.rst

@@ -1,2 +1,5 @@
-.. automodule:: MDAnalysis.lib._distances
+C-distances


Sphinx really wants to have these headings. I don't know why though.

kain88-de · 2017-04-16T15:14:58Z

I also think the cross reference warnings are hard to get rid of. They might be a sphinx bug.

sphinx-doc/sphinx#2549

I'm glad to hear any ideas where they might come from.

kain88-de · 2017-04-16T16:13:36Z

BTW we should take these warnings serious. They result in legitimate wrong docs! For example see the Topology docs in devel and the source. These docs are rendered wrong.

kain88-de · 2017-04-16T16:55:44Z

package/MDAnalysis/analysis/gnm.py

-   output = zip(*C.results)
+    with open("eigenvalues.dat", "w") as outputfile:
+        for item in output[1]:
+            outputfile.write(item + "\n")


using the \n here broke the docs. This has the be set to a raw string.

How did this break the docs???

(Wouldn't this be considered a bug in sphinx?)

No this is normal python behavior. \n is an escape sequence and just like for docs with a latex string inside we need to tell python the string is a raw string.

I hadn't realized that this was Python code inside a doc string...

I don't think you understand me correct. This is a escape sequence in a string. This is a problem for sphinx in any doc string no matter where it stands. That's why we have the raw string hint in the docs for documentation.

kain88-de · 2017-04-16T16:56:24Z

package/MDAnalysis/analysis/gnm.py

+    See Also
+    --------
+    :class:`GNMAnalysis`
+

    .. versionchanged:: 0.16.0


I found by trial and error that the normal .. * sections only work in numpy docs when we leave two blank lines

@kain88-de I love you! This was ruining my life!

I found the explanation for this in an old sphinx issue. But yeah it's a weird bug/feature

Add a note to our doc docs.... keeping this kind of knowledge around is really helpful.

We should also add (if we haven't already) that

numpy See Also does not work well for us

do not use "Examples"/"Example" as a normal section heading unless as a numpy example section

See Also works really fine for me Where have you seen problems.

Were do we document our docs?

In @jbarnoud 's PR #1247 IIRC. EDIT: see #1247 (comment)

Documenting docs: see wiki style guide.

If you find that See Also works then remove the section in the style guide where I recommend against using it.

I think I've already done that.

kain88-de · 2017-04-16T17:11:13Z

package/doc/sphinx/source/documentation_pages/references.rst

-   and K. Lindorff-Larsen. ENCORE: Software for quantitative ensemble
-   comparison. *PLoS Comput Biol*, **11** (2015), e1004415.  doi:
-   `10.1371/journal.pcbi.1004415`_
+[Tiberti2015]_


So here I have a fundamental question. Sphinx doesn't like that we define the reference [Tiberti2015] in several places. So far in my clean up here my strategy was to use one definition in the source code and use that link in the rst files. Is that OK. Should we go a different route?

Bit hackish, we could just uniquely define it, so call it 2015b] to avoid the collision

Actually I like to have just one definition in the docs. Then we are guaranteed to always use the same.

I followed your advice now.

kain88-de · 2017-04-16T17:30:39Z

This PR as of now should half our sphinx errors.

kain88-de · 2017-04-16T19:28:27Z

There is one last class of errors that I just can't fix. For some reason unknown to me currently sphinx sometimes wants to cross reference occurences of n_atoms and Atom. Below are the error messages I get.

/home/max/foss/molecular-dynamics/mdanalysis/mdanalysis/package/MDAnalysis/core/topology.py:docstring of MDAnalysis.core.topology.Topology:None: WARNING: more than one target found for cross-reference u'n_atoms': MDAnalysis.coordinates.DCD.Timestep.n_atoms, MDAnalysis.coordinates.XYZ.XYZReader.n_atoms, MDAnalysis.coordinates.TRZ.Timestep.n_atoms, MDAnalysis.coordinates.base.Timestep.n_atoms, MDAnalysis.coordinates.TRZ.TRZReader.n_atoms, MDAnalysis.core.groups.ResidueGroup.n_atoms, MDAnalysis.core.groups.SegmentGroup.n_atoms, MDAnalysis.core.groups.AtomGroup.n_atoms, MDAnalysis.coordinates.memory.Timestep.n_atoms
/home/max/foss/molecular-dynamics/mdanalysis/mdanalysis/package/MDAnalysis/core/topology.py:docstring of MDAnalysis.core.topology.TransTable:None: WARNING: more than one target found for cross-reference u'n_atoms': MDAnalysis.coordinates.DCD.Timestep.n_atoms, MDAnalysis.coordinates.XYZ.XYZReader.n_atoms, MDAnalysis.coordinates.TRZ.Timestep.n_atoms, MDAnalysis.coordinates.base.Timestep.n_atoms, MDAnalysis.coordinates.TRZ.TRZReader.n_atoms, MDAnalysis.core.groups.ResidueGroup.n_atoms, MDAnalysis.core.groups.SegmentGroup.n_atoms, MDAnalysis.core.groups.AtomGroup.n_atoms, MDAnalysis.coordinates.memory.Timestep.n_atoms
/home/max/foss/molecular-dynamics/mdanalysis/mdanalysis/package/MDAnalysis/lib/NeighborSearch.py:docstring of MDAnalysis.lib.NeighborSearch.AtomNeighborSearch.search:None: WARNING: more than one target found for cross-reference u'Atom': MDAnalysis.core.groups.Atom, MDAnalysis.topology.tpr.obj.Atom, MDAnalysis.core.Timeseries.Atom
docstring of MDAnalysis.lib.formats.libmdaxdr.TRRFile.write:None: WARNING: more than one target found for cross-reference u'n_atoms': MDAnalysis.coordinates.DCD.Timestep.n_atoms, MDAnalysis.coordinates.XYZ.XYZReader.n_atoms, MDAnalysis.coordinates.TRZ.Timestep.n_atoms, MDAnalysis.coordinates.base.Timestep.n_atoms, MDAnalysis.coordinates.TRZ.TRZReader.n_atoms, MDAnalysis.core.groups.ResidueGroup.n_atoms, MDAnalysis.core.groups.SegmentGroup.n_atoms, MDAnalysis.core.groups.AtomGroup.n_atoms, MDAnalysis.coordinates.memory.Timestep.n_atoms
docstring of MDAnalysis.lib.formats.libmdaxdr.XTCFile.write:None: WARNING: more than one target found for cross-reference u'n_atoms': MDAnalysis.coordinates.DCD.Timestep.n_atoms, MDAnalysis.coordinates.XYZ.XYZReader.n_atoms, MDAnalysis.coordinates.TRZ.Timestep.n_atoms, MDAnalysis.coordinates.base.Timestep.n_atoms, MDAnalysis.coordinates.TRZ.TRZReader.n_atoms, MDAnalysis.core.groups.ResidueGroup.n_atoms, MDAnalysis.core.groups.SegmentGroup.n_atoms, MDAnalysis.core.groups.AtomGroup.n_atoms, MDAnalysis.coordinates.memory.Timestep.n_atoms

We really need to fix them since they lead to completely wrong docs as shown in #1312 (comment)

kain88-de · 2017-04-16T19:58:53Z

OK I should have fixed all the errors in the docs now. There seems to be some new pylint errors though.

kain88-de · 2017-04-16T20:06:00Z

Also I plan to rebase the history to something sane once all tests are passing.

I did a lot of changes since this review

kain88-de · 2017-04-17T08:39:05Z

@richardjgowers I'm so far done now.

the history is rebased
Mathjax cdn has been updated
doc warnings are errors

It would be nice if someone could review it again to make sure everything is still alright. The PR touches a lot of things.

kain88-de · 2017-04-17T08:42:36Z

package/MDAnalysis/analysis/contacts.py

@@ -536,7 +536,6 @@ def q1q2(u, selection='all', radius=4.5,
 # until then it remains because we don't have the functionality
 # elsewhere.

-@deprecate(new_name="Contacts", message="This class will be removed in 0.17")


having the deprecate at the top of the class made sphinx think this is a function and not a class. Moving it to the __init__ function fixed that. Bonus we can also use the recent sphinx version now.

kain88-de · 2017-04-17T14:53:21Z

tests all look like they pass. It's just the one with coverage that always freezes at the end.

jbarnoud · 2017-04-17T17:13:26Z

On 04/17/2017 07:11 PM, Max Linke wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In package/MDAnalysis/analysis/gnm.py <#1312 (comment)>: > - .. SeeAlso:: :class:`GNMAnalysis` + Notes + ----- + The `MassWeight` option does not perform a true mass weighting but + weighting by the number of atoms in each residue; the name of the parameter + exists for historical reasons and will be removed in 0.17.0. Until then, + setting `MassWeight` to anything but ``None`` will override `weights`. + + See Also + -------- + :class:`GNMAnalysis` + .. versionchanged:: 0.16.0 |See Also| works really fine for me Where have you seen problems. Were do we document our docs? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1312 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUWuhIIva06OQqbOblvMNGMg4XOMnYdks5rw51QgaJpZM4M-pn0>.

I had problems, but from what I see, the double new line should fix them.

orbeckst · 2017-04-17T17:25:10Z

Generally speaking, if one writes docs one actually needs to look at the NumPy doc format and reST to write syntactically correct docs. Not everything that looks reasonable actually works. Probably the most useful page for me is the NumPy example

I just added these links to https://github.com/MDAnalysis/mdanalysis/wiki/Style-Guide#writing-docstrings

BTW we should take these warnings serious. They result in legitimate wrong docs! For example see the Topology docs in devel and the source. These docs are rendered wrong.

orbeckst

Great, thanks. The only reason why I am blocking is because of the handling of the box keyword (see comments).

orbeckst · 2017-04-17T17:30:01Z

package/MDAnalysis/analysis/gnm.py


    .. versionchanged:: 0.16.0
       Made :meth:`generate_output` a private method :meth:`_generate_output`.

    .. deprecated:: 0.16.0
       Instead of ``MassWeight=True`` use ``weights="size"``.
+


Is a trailing blank empty line necessary?

(I don't care if it's there or not if it's aesthetics but would want to know if there are functional/weird reasons.)

Not sure. Might be a leftover from some early tries fixing the sphinx warnings.

orbeckst · 2017-04-17T18:28:46Z

package/MDAnalysis/core/topology.py

@@ -151,8 +151,12 @@ class TransTable(object):

    Parameters
    ----------
-    n_atoms, n_residues, n_segments : int


This is a doc pattern that needs to be unlearned by us all.

It is valid for numpy docs. But Napoleon doesn't work well with it.

Too bad, I liked this pattern. It avoided quite a lot of repetition.

I think it still works in some places. We used it more often. But there also isn't a good way to look at the intermediate rEst to know what exactly is going on.

orbeckst · 2017-04-17T18:29:31Z

package/MDAnalysis/core/topology.py

@@ -165,7 +169,7 @@ class TransTable(object):

    Attributes
    ----------
-    n_atoms, n_residues, n_segments : int
+    `n_atoms`, n_residues, n_segments : int


These should be individual entries, shouldn't they?

Even if escaping the first one works, it seems inconsistent with all the other docs that do not have backtics on variable names or argument names in these lists.

orbeckst · 2017-04-17T18:32:26Z

package/MDAnalysis/core/topologyattrs.py

@@ -1362,7 +1362,7 @@ class Bonds(_Connection):

    Must be initialised by a list of zero based tuples.
    These indices refer to the atom indices.
-        Eg:  [(0, 1), (1, 2), (2, 3)]
+    Eg:  [(0, 1), (1, 2), (2, 3)]


"E.g.," contains periods and is followed by a comma. think it makes more sense to show the list of tuples as code.

E.g., ` [(0, 1), (1, 2), (2, 3)]`

orbeckst · 2017-04-17T18:32:46Z

package/MDAnalysis/core/topologyattrs.py

@@ -1416,8 +1416,7 @@ class Angles(_Connection):
    """Angles between three atoms

    Initialise with a list of 3 long tuples
-    Eg:
-      [(0, 1, 2), (1, 2, 3), (2, 3, 4)]
+    Eg: [(0, 1, 2), (1, 2, 3), (2, 3, 4)]


"E.g.," contains periods and is followed by a comma. think it makes more sense to show the list of tuples as code.

orbeckst · 2017-04-17T18:34:36Z

package/MDAnalysis/lib/NeighborSearch.py

@@ -68,7 +68,7 @@ def search(self, atoms, radius, level='A'):

        Parameters
        ----------
-        atoms : AtomGroup or Atom
+        atoms : AtomGroup or `Atom`


Would Atom be recognized if the types were separated by comma and not "or", i.e.,

atoms : AtomGroup, Atom

Nope it doesn't

But tellling it exactly the class we mean works and it links correctly. That's not bad.

orbeckst · 2017-04-17T18:37:51Z

package/MDAnalysis/lib/util.py

+        reset : boolean
+            start the stream from the beginning (either :meth:`reset` or :meth:`seek`)
+            when the class instance is constructed
+        close : booelan


boolean (typo)

Also note that bool gets linked to bool whereas boolean is left as text.

orbeckst · 2017-04-17T18:39:11Z

package/MDAnalysis/lib/util.py

+        Notes
+        -----
+        By default, this stream will *not* be closed by :keyword:`with` and
+        :meth:`close` (see there) unless the *close* keyword is set to


Use backtics for name references:

*close* --> `close`

orbeckst · 2017-04-17T18:45:10Z

package/MDAnalysis/topology/guessers.py

@@ -203,7 +200,8 @@ def guess_bonds(atoms, coords, **kwargs):

    lower_bound = kwargs.get('lower_bound', 0.1)

-    box = kwargs.get('box', None)
+    if box:


This is bad and will typically fail for box being an array with ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

You should use

if box is not None:

or similar.

Or do a try/except.

Make sure to assign None in all cases where there is no array. Downstream code might actually rely on box = None and not handle box = False or box = 0 correctly.

The is not None check is enough. That will reflect the old behavior and the numpy asarray call later ensures us the we have an array.

orbeckst · 2017-04-17T18:48:47Z

How does this PR interact with @jbarnoud 's #1247 ?

Ties in with #1277.

jbarnoud · 2017-04-17T19:28:41Z

On 04/17/2017 08:48 PM, Oliver Beckstein wrote: How does this PR interact with @jbarnoud <https://github.com/jbarnoud> 's #1247 <#1247> ? Ties in with #1277 <#1277>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1312 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUWulAbkxaGOCYBALv-UzCo7g_NGjyDks5rw7QQgaJpZM4M-pn0>.

My #1247 is broken. I'll rework it on top of this commit.

kain88-de · 2017-04-17T19:39:57Z

How does this PR interact with @jbarnoud 's #1247 ?

Ties in with #1277.

No idea. My guess it that it breaks the other doc PR's. But I think tha's a fair price to pay since this will enable them to have working sphinx docs.

This also activates the .. TODO:: sections to be legal

use more ci-helpers goodies.

They are triggered now. I don't know how to fix them. I think they are triggered now because pylint used to be tripped by errors in the docs. Why that happened I'm not sure.

The original cdn will be taken down on 30 April 2017

kain88-de · 2017-04-17T19:55:03Z

@orbeckst fixed your remarks

kain88-de commented Apr 16, 2017

View reviewed changes

kain88-de force-pushed the doc-warnings branch from be5e87b to 6ac60fe Compare April 16, 2017 16:55

kain88-de commented Apr 16, 2017

View reviewed changes

richardjgowers previously approved these changes Apr 16, 2017

View reviewed changes

kain88-de force-pushed the doc-warnings branch 2 times, most recently from 74dffc4 to 29a96b7 Compare April 17, 2017 08:23

kain88-de commented Apr 17, 2017

View reviewed changes

kain88-de force-pushed the doc-warnings branch from 712dae5 to da0c1c3 Compare April 17, 2017 11:05

orbeckst requested changes Apr 17, 2017

View reviewed changes

kain88-de added 5 commits April 17, 2017 21:54

Fix doc warnings in MDAnalysis package

dfad88c

fix errors in rst files

317dedf

This also activates the .. TODO:: sections to be legal

turn doc warnings into errors

322bbb9

update travis file

d249fd7

use more ci-helpers goodies.

remove pylint warnings

373c1fb

They are triggered now. I don't know how to fix them. I think they are triggered now because pylint used to be tripped by errors in the docs. Why that happened I'm not sure.

kain88-de added 2 commits April 17, 2017 21:54

update mathjax cdn

7a76828

The original cdn will be taken down on 30 April 2017

update CHANGELOG

c9f76b2

kain88-de force-pushed the doc-warnings branch from da0c1c3 to c9f76b2 Compare April 17, 2017 19:54

orbeckst approved these changes Apr 17, 2017

View reviewed changes

kain88-de merged commit ca30efa into develop Apr 17, 2017

kain88-de deleted the doc-warnings branch April 18, 2017 10:05

		@@ -1,2 +1,5 @@
		.. automodule:: MDAnalysis.lib._distances
		C-distances

fix sphinx warnings #1312

fix sphinx warnings #1312

Conversation

kain88-de commented Apr 16, 2017 • edited Loading

PR Checklist

Choose a reason for hiding this comment

kain88-de commented Apr 16, 2017

kain88-de commented Apr 16, 2017

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

orbeckst Apr 17, 2017 • edited Loading

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

kain88-de commented Apr 16, 2017

kain88-de commented Apr 16, 2017

kain88-de commented Apr 16, 2017

kain88-de commented Apr 16, 2017

kain88-de commented Apr 17, 2017

Choose a reason for hiding this comment

kain88-de commented Apr 17, 2017

jbarnoud commented Apr 17, 2017 via email

orbeckst commented Apr 17, 2017

orbeckst 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst commented Apr 17, 2017

jbarnoud commented Apr 17, 2017 via email

kain88-de commented Apr 17, 2017

kain88-de commented Apr 17, 2017

kain88-de commented Apr 16, 2017 •

edited

Loading

orbeckst Apr 17, 2017 •

edited

Loading