Some Modules improperly iterate over trajectory, documentation error

[jdetle]

link to groups conversation here: https://groups.google.com/forum/#!topic/mdnalysis-devel/uoAoJz8aKtA

default start, stop, step should always be None, None, None, trajectory.check_slice_indices(None, None, None) returns 0, n_frames, 1, as it should.

Expected behaviour

As an example:
In rms/RMSF default stop value=-1 instead of None. This is meant to run over an entire trajectory.

Actual behaviour

RMSF stops on the frame before the last. This isn't apparent because the np array does some broadcasting over all values in the arras. RMSF should not throw a NameError if operated on a single frame as well.

Code to reproduce the behaviour

import MDAnalysis as mda
import MDAnalysis.analysis.rms as rms
from MDAnalysisTest.datafiles import PSF, DCD
u = mda.Universe(PSF, DCD)
test_1 = rms.RMSF(u)
test_2 = rms.RMSF(u, stop=None)
test_1.run()
test_2.run()

print test_1._rmsf == test_2._rmsf
(will be false)

Mistakenly used and improperly documented in DCD:

• wrong default : line 501
• wrong documentation : line 508 (NOT INCLUSIVE)
• wrong default : line 527
• wrong docs : line 533

wrong defaults

• analysis/rms.py (RMSF)

Improperly documented in:

• coordinates/base.py : check_slice_indices ~line 1226
• analysis/contacts.py : lines 402 and 499
• analysis/diffusionmap.py: line 232
• analysis/polymer.py : line 64

[orbeckst]

@jdetle, I edited your excellent issue report (added check list marks, added section "wrong defaults", and added missing word "not").

[jdetle]

@orbeckst Do you happen to know where the C code for _dcdmodule exist prior to compilation?

[orbeckst]

Welcome to MDAnalysis hell archaeology 101:

• coordinates/src/dcd.c becomes the module and includes the VMD DCD reader code via include files. This piece of code directly generates CPython objects and is the major stumbling block in getting MDA to Py 3 – see support python3 #260 and in particular DCD Module doesn't support Python 3 #659. (In all fairness, it's also probably the nucleation point from which MDAnalysis grew and hearkens back to a time when cython didn't exist, pyrex was a new thing, and Python 2.4 was current.)
• coordinates/include/ contains *.h files which form a "library" of C-code (they are not just headers, they are meant to be included instead of linked)
• coordinates/timeseries.pyx is essentialy again a DCD reader, but written in pyrex/cython, and still using the include files.

[jdetle]

@orbeckst I laughed. It looks like we will have to change the dcd code to reflect the proper slicing behavior. I assume I have to do gcc dcd.c ../_dcdmodule to get the code to compile correctly?

[orbeckst]

On 27 Jul, 2016, at 12:35, John Detlefs [email protected] wrote:

It looks like we will have to change the dcd code to reflect the proper slicing behavior. I assume I have to do gcc dcd.c ../_dcdmodule to get the code to compile correctly?

1. write a test case
2. make sure it FAILS with the current code
3. fix the C/cython code
4. rebuild MDAnalysis (pip install package/ or python setup.py – whatever you normally do) – setup.py knows how to deal with the C code
5. run your test until you get it to pass

[jdetle]

@orbeckst is there an easy way to print debugging statements? Currently printf is causing a segfault.
EDIT: I was using printf(' '); instead of printf(" ") (Ahh, C...);

[jdetle]

/* 
 * Skip past a timestep.  If there are fixed atoms, this cannot be used with
 * the first timestep.  
 * Input: fd - a file struct from which the header has already been read
 *        natoms - number of atoms per timestep
 *        nfixed - number of fixed atoms
 *        charmm - charmm flags as returned by read_dcdheader
 * Output: 0 on success, negative error code on failure.
 * Side effects: One timestep will be skipped; fd will be positioned at the
 *               next timestep.
 */
static int skip_dcdstep(fio_fd fd, int natoms, int nfixed, int charmm, int numstep);

😩

[orbeckst]

It builds character to write C code from time to time.

(Regarding #914 (comment) please explain for the slower folks in the audience (=me), please.)

[jdetle]

Sorry I was just whining that it managed to document 4 out of the 5 variables...