Skip to content
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

Cleaned up Reader classes #317

Merged
merged 1 commit into from
Jun 16, 2015
Merged

Cleaned up Reader classes #317

merged 1 commit into from
Jun 16, 2015

Conversation

richardjgowers
Copy link
Member

In preparation for many Reader improvements ( #239 #314 ), did a clean up of how Reader.__iter__ works

Removed custom __iter__ from all but coordinates.xdrfile.core (XTC & TRR formats)

Removed many custom _sliced_iter implementations, can use base version

base.Reader now has __iter__ method

base.Reader.rewind now doesn't require seeking access, instead
_reopens and calls next

Removed custom __iter__ from all but xdrfile (XTC & TRR)

Removed many custom _sliced_iter implementations, can use base version

base.Reader now has __iter__ method

base.Reader rewind now doesn't require seeking access, instead
reopens and calls next
@richardjgowers richardjgowers self-assigned this Jun 16, 2015
@richardjgowers richardjgowers added this to the 0.11 milestone Jun 16, 2015
self._reset_dcd_read()

def iterDCD():
for i in xrange(0, self.numframes, self.skip): # FIXME: skip is not working!!!
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've just noticed this... does DCD skip do anything?

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the DCDReader code, we just set self.skip = 1.
I think it really ought be called "step" but in the underlying dcd.c code, it's called skip. In principle the DCD C code should allow frame skipping at the C level with skip_dcdstep() but I just found the comment "XXX this needs to be changed"....

    // Figure out how many steps to skip
    numskip = skip - (dcd->setsread % skip) - 1;
    /* XXX this needs to be changed */
    rc = skip_dcdstep(dcd->fd, dcd->natoms, dcd->nfixed, dcd->charmm, numskip);

It might be related to having the possibility of fixed atoms in a DCD or it might simply not work as expected and that's why we seem to have effectively disabled skipping at the C level. I'm sorry, I don't remember.

It seems to work, though, because:

import MDAnalysis; from MDAnalysis.tests.datafiles import PSF, DCD
u = MDAnalysis.Universe(PSF,DCD)
u.trajectory.skip = 10
frames = [ts.frame for ts in u.trajectory]
print(frames)

gives

[10, 20, 30, 40, 50, 60, 70, 80, 90]

as expected,

@orbeckst
Copy link
Member

@richardjgowers , looks fine. I'll merge it.

We're not loosing anything with regard to "DCD skip" but we can see if C-level skip can be used to speed up special cases of the the sliced iterator (although: maybe benchmark first on something big if it is faster than the python based implementation that uses computed seeks, which is pretty much the same what the C-version does).

orbeckst added a commit that referenced this pull request Jun 16, 2015
@orbeckst orbeckst merged commit fbceab4 into develop Jun 16, 2015
@richardjgowers
Copy link
Member Author

Ok thanks for clarifying. I'd expect that a Python file seek will be comparable too.

@richardjgowers richardjgowers deleted the feature-unified_iter branch June 19, 2015 14:40
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.

2 participants