transition analysis docs to numpy style by orbeckst · Pull Request #1240 · MDAnalysis/mdanalysis

orbeckst · 2017-03-10T00:09:31Z

Use this WIP PR to accumulate doc fixes for analysis. When we have enough we can merge or squash-merge. I didn't want to do a proper PR for a single simple reST fix...

Changes made in this Pull Request:

doc fixes in analysis (eg SeeAlso, Example sections, ...)
transition analysis docs to numpy style

Note that @jbarnoud has been tackling all other docs in PR #1247.

PR Checklist

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

orbeckst · 2017-03-24T03:59:26Z

I cleaned up some code in analysis.gnm and apparently broke things. Will need to look into this.

orbeckst · 2017-03-24T04:13:16Z

fixed gnm issue, rebased and force-pushed

orbeckst · 2017-03-24T17:05:04Z

... and travis exceeded time so I restarted the job. (We really need to reduce the test time #1191 )

orbeckst · 2017-03-24T19:33:48Z

Full test exceeded time again. I'll try rolling back whatever small code changes I made in analysis.gnm.

(Locally

./mda_nosetests -v ./analysis/test_gnm.py

takes 122.4 s on a single 3.1 GHz i7 core with most of the time spent on test_gnm.TestGNM.test_closeContactGNMAnalysis.)

orbeckst · 2017-03-30T08:41:04Z

package/MDAnalysis/analysis/gnm.py

                if jcounter > icounter and _dsq(positions[icounter], positions[jcounter]) <= cutoffsq:
                    iresidue, jresidue = residue_index_map[icounter], residue_index_map[jcounter]
-                    if self.MassWeight:
-                        contact = 1.0 / (len(self.ca.residues[iresidue].atoms) * len(self.ca.residues[jresidue].atoms)) ** 0.5


This line was responsible for the huge amount of time that the GNM test TestGNM.test_closeContactGNMAnalysis took.

Can you push your fix to a new branch separate from this?

also this logic appears also in the other class in this file. It would be nice if you can change it their too.

Will fix elsewhere; apparently only mattered in the instance were I fixed it because many more atoms were used.

I can try to disentangle but their were also some cleanups in gnm in an earlier commit. I will look at a local interactive rebase and see if I can merge these commits.

Er, actually, where did you see another instance of this problem? GNMAnalysis.generate_kirchoff() does not use custom weights. I am not sure where else this needs fixing.

Looks like I misremembered. I thought both classes use the custom weights.

See PR #1272 – all analysis.gnm changes were removed from this PR.

@richardjgowers / @dotsdl would you expect a lookup such as len(self.ca.residues[iresidue].atoms to be slow? In particular, is self.ca.residues the bottle neck?

So assuming self.ca is an AtomGroup,

ca.residues will be performing a np.unique on all the residue indices in the group,

residues[iresidue] is just slicing a numpy array

residue.atoms will be looking up a single entry in a residue->atoms table.

So nothing too offensively slow from what I can see, but you might be able to calculate them all ahead of time (as these memberships won't change) if you're looping over these things a lot.

orbeckst · 2017-03-30T08:41:56Z

Optimized a single line in analysis.gnm.closeContactGNMAnalysis.generate_kirchhoff() and locally reduced the run time of the longest test TestGNM.test_closeContactGNMAnalysis by about 4-5 fold.

(Also rebased and force pushed.)

kain88-de · 2017-03-30T09:46:59Z

package/MDAnalysis/analysis/gnm.py

        cutoffsq = self.cutoff ** 2

+        # cache sqrt of residue sizes (slow) so that sr[i]*sr[j] == sqrt(r[i]*r[j])
+        sqrt_res_sizes = np.sqrt([r.atoms.n_atoms for r in  self.ca.residues]) if self.MassWeight else None


Btw is that really a weights defined by the mass? It looks only like a estimate of the actual mass. Would size be a better name?

Since you are refactoring this code already. It be nice if this variable was renamed weights and then have options None or 'size'.

It's not really weight but that's what was in the original code.

I'll rename the variable but leave the kwarg as it is but add a note.

See e335c37

Done in PR #1272

orbeckst · 2017-03-30T10:41:37Z

I moved the GNM improvements to separate PR #1272 and rebased and force-pushed.

orbeckst · 2017-03-30T16:06:09Z

Yes, that's what's being done now. Speed up of about 5 times.

…

-- Oliver Beckstein email: orbeckst@gmail.com

Am Mar 30, 2017 um 5:43 schrieb Richard Gowers ***@***.***>: but you might be able to calculate them all ahead of time (as these memberships won't change) if you're looping over these things a

orbeckst · 2017-03-31T09:12:18Z

rebased and force pushed

orbeckst · 2017-03-31T11:10:15Z

All analysis docs are now numpy style.

orbeckst · 2017-03-31T20:34:38Z

Somone has to review it, otherwise it can't be merged...

orbeckst · 2017-03-31T20:38:19Z

package/MDAnalysis/analysis/hbonds/hbond_analysis.py

 .. _`10.1002/prot.340090204`: http://dx.doi.org/10.1002/prot.340090204


-Example


Just fyi: with numpy docs, never use Example or Examples in a normal section context. sphinx napoleon rewrites it. So you have to ne a bit creative with labelling sections in the main part of the page.

kain88-de

I only found minor issues skimming over this. Good work!

kain88-de · 2017-03-31T21:56:49Z

package/MDAnalysis/analysis/nuclinfo.py


-    .. NOTE:: If failure occurs be sure to check the segment identification.
+
+    .. note:: If failure occurs be sure to check the segment identification.


Big letter N? Why Note use the numpy note section?

Changed to Notes section (rebased so it now shows up in commit 056f097 )

kain88-de · 2017-03-31T21:59:30Z

package/MDAnalysis/analysis/psa.py

     >>> hausdorff_wavg(P,Q[::-1]) # weighted avg hausdorff dist w/ Q reversed
     2.5669644353703447
+
+    Notes


Is it with the trailing s?

Yes, its Notes as in https://github.com/numpy/numpy/blob/master/doc/example.py

kain88-de · 2017-03-31T22:05:21Z

package/MDAnalysis/analysis/nuclinfo.py

+

-    .. NOTE:: If failure occurs be sure to check the segment identification.
+    .. Note:: If failure occurs be sure to check the segment identification.


Why not the numpy notes section

Changed to Notes section (rebased so it now shows up in commit 056f097 )

Do not use Examples as a heading UNLESS inside a function/class doc because the NumPy reST parser changes it to a rubric heading. This breaks document structure.

@tylerjereddy

- converted all docs to numpy style - added additional references - see also: @tylerjereddy 's scipy.spatial.distance.directed_hausdorff()

- analysis.align: formatting fixes - analysis.contacts: formatting fixes - analysis.diffusionmap: formatting fixes and section headers (cannot use 'Examples' as a normal section header because it is rewritten by sphinx.ext.napoleon as a rubric) - analysis.distances: numpyfied - analysis.hbonds.hbond_analysis: numpyfied

- numpified docs - removed kwargs start and end for resid selection from def helanal_main() and helanal_trajectory() because this can be easily done inside the selection string and neither start nor end are used further in the code. helanal_trajectory() uses the resid of the first and last residue extensively for reporting so we now get these resids from the selection itself.

orbeckst · 2017-04-01T16:34:33Z

Incorporated @kain88-de 's suggestions, rebased into the appropriate commit, and rewrote some of the commit messages. I think I am done.

When @jbarnoud finishes PR #1247 then we will have transitioned all our docs to numpy style.

kain88-de · 2017-04-18T10:14:35Z

package/MDAnalysis/analysis/align.py

-    :func:`sequence_alignment`, which does not require external
-    programs.
+
+    .. SeeAlso::


Any idea how to change those automatically to the old style? My first try find . -name '*py' -exec sed -i "s/.. SeeAlso::/See Also \n--------\n/" {} \; doesn't work. It can't deal with potential indentation of the initial see also paragraph.

I had a quick look and I ended up with this:

sed -re 's/^(( *).. SeeAlso::( *))/\2See Also\n\2--------\n\2/g' package/MDAnalysis/lib/util.py | less

You need the -r or you will have to escape all the parentheses, and you may not have the \2 syntax. The \2 is there to report the indentation.

@jbarnoud

* replace .. SeeAlso:: with numpy section * fix obvious formatting errors * addressed comments from @jbarnoud * fix rendering issues * some more usability changes * add ExtendedPDBReader to docs * fix last link issue I used the following command to replace all occurrences. find . -name '*py' -exec sed -rie 's/^(( *).. SeeAlso::( *))/\2See Also\n\2--------\n\2/g' {} \; Following a comment of @jbarnoud at #1240 (comment)

orbeckst added Component-Docs Work in progress labels Mar 10, 2017

orbeckst force-pushed the doc-fixes-2 branch from 8b1569b to 8b988d0 Compare March 23, 2017 10:21

orbeckst added the help wanted label Mar 23, 2017

orbeckst changed the title ~~[WIP] various doc fixes~~ [WIP] transition analysis docs to numpy style Mar 23, 2017

orbeckst force-pushed the doc-fixes-2 branch from 8b988d0 to a8b41cb Compare March 24, 2017 04:12

orbeckst force-pushed the doc-fixes-2 branch from a8b41cb to 9ad0f3a Compare March 30, 2017 08:37

orbeckst commented Mar 30, 2017

View reviewed changes

kain88-de reviewed Mar 30, 2017

View reviewed changes

orbeckst mentioned this pull request Mar 30, 2017

analysis.gnm: optimizations and doc/code cleanup #1272

Merged

4 tasks

orbeckst force-pushed the doc-fixes-2 branch from e335c37 to a18d05c Compare March 30, 2017 10:40

orbeckst force-pushed the doc-fixes-2 branch from a18d05c to ee23591 Compare March 31, 2017 09:10

orbeckst force-pushed the doc-fixes-2 branch from adb369e to d35c6b8 Compare March 31, 2017 10:36

orbeckst removed help wanted Work in progress labels Mar 31, 2017

orbeckst changed the title ~~[WIP] transition analysis docs to numpy style~~ transition analysis docs to numpy style Mar 31, 2017

orbeckst added the needs review label Mar 31, 2017

orbeckst added this to the 0.16.0 milestone Mar 31, 2017

orbeckst commented Mar 31, 2017

View reviewed changes

kain88-de approved these changes Mar 31, 2017

View reviewed changes

orbeckst added 9 commits April 1, 2017 09:16

analysis.density: fixed top level Example section in docs

e54ff89

Do not use Examples as a heading UNLESS inside a function/class doc because the NumPy reST parser changes it to a rubric heading. This breaks document structure.

analysis.psa: doc fixes

3da846f

- converted all docs to numpy style - added additional references - see also: @tylerjereddy 's scipy.spatial.distance.directed_hausdorff()

analysis.leaflet: numpified docs and added test for optimize_cutoff()

d396244

analysis.legacy.x3dna: numpified docs and use OrderedDict

ff5f7d2

analysis.nuclinfo: numpyfied docs

056f097

analysis.rdf: fixed numpy reST

2e2017b

analysis.waterdynamics: numpyfied docs (TODO: PEP8)

bec81f2

orbeckst force-pushed the doc-fixes-2 branch from 012085d to bec81f2 Compare April 1, 2017 16:22

orbeckst mentioned this pull request Apr 1, 2017

convert all docs to numpy style #1277

Closed

3 tasks

kain88-de merged commit 3aecf99 into develop Apr 2, 2017

kain88-de deleted the doc-fixes-2 branch April 2, 2017 12:27

kain88-de reviewed Apr 18, 2017

View reviewed changes

kain88-de mentioned this pull request Apr 18, 2017

replace .. SeeAlso:: with numpy section #1314

Merged

4 tasks

		.. _`10.1002/prot.340090204`: http://dx.doi.org/10.1002/prot.340090204


		Example


		.. NOTE:: If failure occurs be sure to check the segment identification.

		.. note:: If failure occurs be sure to check the segment identification.

Conversation

orbeckst commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

orbeckst commented Mar 24, 2017

Uh oh!

orbeckst commented Mar 24, 2017

Uh oh!

orbeckst commented Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orbeckst commented Mar 24, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orbeckst Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

orbeckst commented Mar 30, 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!

orbeckst commented Mar 30, 2017

Uh oh!

orbeckst commented Mar 30, 2017 via email

Uh oh!

orbeckst commented Mar 31, 2017

Uh oh!

orbeckst commented Mar 31, 2017

Uh oh!

orbeckst commented Mar 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kain88-de 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!

orbeckst commented Apr 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

orbeckst commented Mar 10, 2017 •

edited

Loading

orbeckst commented Mar 24, 2017 •

edited

Loading

orbeckst Mar 30, 2017 •

edited

Loading