Slicing fixes #916

jdetle · 2016-07-27T01:22:19Z

Changes made in this Pull Request:

Changed coordinates/base.py to reflect true behavior of check_slice_indices
Changed DCD correl and timeseries documentation and defaults to reflect behavior of start stop step
Changed diffusion map docs to reflect the behavior of default start, stop, step
Changed density docs to reflect the behavior of default start, stop, step
Changed contact analysis docs to reflect the behavior of default start, stop, step
Changed RMSF docs to reflect behavior of default start, stop, step
Changed RMSF testing to reflect proper value.
Changed DCD reader source for timeseries creation. This included reformatting some code to make it look nicer, adding a helper variable to keep track of the number of frames and changing the way skipping is done.
Changed DCD
Added known failures for backwards steps steps.
PR Checklist
- Tests?
- Docs?
- CHANGELOG updated?
- Issue raised/referenced?

TODO

Deprecate skip for step in DCD correl and timeseries
Add more tests for slice checking
Update docs to be more clear regarding inclusivity and exclusivity of slicing.

LINE CHANGES IN dcd.c

line 440
[line 470](https://github.com/jdetle/mdanalysis/blob/1c6c24124c1640294b65e6bc27f4cf8c09f962db/package/M
DAnalysis/coordinates/src/dcd.c#L470)

frame iteration lines

coveralls · 2016-07-27T01:46:15Z

Coverage increased (+0.003%) to 83.128% when pulling 09e8b5b on jdetle:slicing_fixes into e5c98de on MDAnalysis:develop.

jdetle · 2016-07-27T02:13:01Z

@MDAnalysis/coredevs a review please?

richardjgowers · 2016-07-27T08:51:35Z

Looks good, but because you've changed a kwarg I think you'll need to document that skip doesn't work and step should be used.

orbeckst · 2016-07-27T18:25:55Z

package/MDAnalysis/analysis/contacts.py

        stop : int, optional
-            Last frame of trajectory to analyse, Default: -1
+            Frame index to stop analysis. Default: None becomes
+            n_frames. Iteration stops *before* this frame number.


, which means that the trajectory would be read until the end (because the first frame has index 0 and the last frame has index n_frames - 1.

Right, that is unclear here and I'll fix it.

On 27/07/16 20:25, Oliver Beckstein wrote:

In package/MDAnalysis/analysis/contacts.py
#916 (comment):

stop : int, optional

Last frame of trajectory to analyse, Default: -1

Frame index to stop analysis. Default: None becomes

n_frames. Iteration stops _before_ this frame number.

, which means that the trajectory would be read until the end (because
the first frame has index 0 and the last frame has index n_frames - 1.

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
https://github.com/MDAnalysis/mdanalysis/pull/916/files/79b1582267ece8ea14f27fd4bed37ef187d95544#r72495216,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUWuuLApCu8e94JQMbJNZsA4rp733hgks5qZ6K0gaJpZM4JVv0L.

This is an odd behavior. The default should not be -1, it should be
None. Why would we want to stop before the end of the trajectory?

Hence the pull request?

@jdetle Sorry, I read the comment from a mail notification, and I thought the code abstract was the new snippets. My bad.

No problem.

orbeckst · 2016-07-27T18:32:38Z

See comments in code. Needs a closer look how this interacts (or possibly solves) #818.

jdetle · 2016-07-27T23:59:58Z

I have slain the beast grendel (#818).

jdetle · 2016-07-28T00:08:05Z

( I am pretty proud of myself right now in case any of you were wondering...)

coveralls · 2016-07-28T00:27:04Z

Coverage increased (+0.04%) to 83.171% when pulling 1c6c241 on jdetle:slicing_fixes into e5c98de on MDAnalysis:develop.

coveralls · 2016-07-28T00:58:57Z

Coverage increased (+0.04%) to 83.171% when pulling 1c6c241 on jdetle:slicing_fixes into e5c98de on MDAnalysis:develop.

orbeckst · 2016-07-28T01:06:52Z

package/MDAnalysis/coordinates/DCD.py

@@ -514,7 +515,11 @@ def timeseries(self, asel, start=None, stop=None, step=None, format='afc'):
               where the shape is (frame, number of atoms,
               coordinates)
        """
-        start, stop, skip = self.check_slice_indices(start, stop, step)
+        if skip is not None:


Here add a deprecation warning, skip to be removed in 1.0.

Also add a .. versionchanged:: 0.16.0 that explains how skip is now step and behaves differently; you can add a link to issue #818.

orbeckst · 2016-07-28T01:16:02Z

Can you squash b7b58ad and 1c6c241 into one commit?

2a5fc32 is really hard to read because of the formatting changes. What happened in this commit?

Will this PR also close #818? If so, add to the last commit touching dcd.c the line "closes #818".

jdetle · 2016-07-28T02:41:44Z

Hi @orbeckst. Sorry, I reformatted the C code because it was hard for me to read. I can certainly squash some commits. I also have yet to address the comments you previously made so I will finish that either tonight or tomorrow.

jdetle · 2016-07-28T02:42:17Z

It will close #818, yes.

orbeckst · 2016-07-28T04:18:26Z

When you reformat then make that a single commit and say in the commit message that this is only formatting and no functional changes.

This makes it a lot easier to follow diffs.

Oliver Beckstein
email: [email protected]

Am Jul 27, 2016 um 19:41 schrieb John Detlefs [email protected]:

Hi @orbeckst. Sorry, I reformatted the C code because it was hard for me to read. I can certainly squash some commits. I also have yet to address the comments you previously made so I will finish that either tonight or tomorrow.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

jdetle · 2016-07-28T04:31:59Z

Unfortunately that commit both reformatted and changed things. My best guess of fixing things would be git rebase -i HEAD~10, pick the commit before the changes to the c code with the edit command, and then make a new commit here in this step of the rebase where I do all the formatting. If I do git rebase --- continue from here the diff should have less problems I think.

jdetle · 2016-07-28T04:36:58Z

Merrrgggg that doesn't work.

coveralls · 2016-07-28T06:07:02Z

Coverage increased (+0.005%) to 83.131% when pulling d5229f7 on jdetle:slicing_fixes into e5c98de on MDAnalysis:develop.

richardjgowers · 2016-07-28T10:48:41Z

testsuite/MDAnalysisTests/coordinates/test_dcd.py

@@ -137,6 +137,15 @@ def test_volume(self):
                            err_msg="wrong volume for unitcell (no unitcell "
                            "in DCD so this should be 0)")

+    def test_timeseries_slicing(self):


So this is a good test, but I'd turn it into a generator.. so something like:

def test_timeseries_slicing(): for start, stop, step in ( (None, None, 10), # this is your current test (1, None, 10), # this tests non zero start point etc: yield self._check_timeseries, start, stop, step def _check_timeseries(self, start, stop, step): self.u = mda.U ts = timeseries() ts_skip = timeseries(self.u.atoms, start, stop, step) assert_array_almost_equal([ts[:, start:stop:step], ts_skip, 5)

orbeckst · 2016-07-28T16:54:20Z

Yes, splitting the commit into formatting and changes would be awesome for readability. It's also important for any efforts to move to Py3: I this gets rewritten we need to know what was changed.

I think your git rebase with edit will work for splitting the commit with multiple add/commit during the edit step. Maybe you need a reset, too, in there somewhere. S/O has a good answer on git commit splitting.

Oliver Beckstein
email: [email protected]

Am Jul 27, 2016 um 21:31 schrieb John Detlefs [email protected]:

Unfortunately that commit both reformatted and changed things. My best guess of fixing things would be git rebase -i HEAD~10, pick the commit before the changes to the c code with the edit command, and then make a new commit here in this step of the rebase where I do all the formatting. If I do git rebase --- continue from here the diff should have less problems I think

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

jdetle · 2016-07-28T16:57:42Z

I tried to do it with a simple rebase and things did not work super well. For what its worth I included the lines of changed code in the first comment in this PR.

coveralls · 2016-07-28T18:28:39Z

Coverage decreased (-0.03%) to 83.1% when pulling 9141bd1 on jdetle:slicing_fixes into e5c98de on MDAnalysis:develop.

jdetle · 2016-07-28T22:50:30Z

@richardjgowers Could you take some time to look at my test generator and tell me what I'm doing wrong? I suspect the tests aren't running as they should because the tests took the same time before and after adding the generator.

orbeckst · 2016-07-31T18:21:26Z

package/MDAnalysis/coordinates/dcdtimeseries.pyx

            rc = skip_dcdstep(dcd.fd, dcd.natoms, dcd.nfixed, dcd.charmm, numskip)
            if (rc < 0):
                raise IOError("Error skipping frame from DCD file")
-            dcd.setsread = dcd.setsread + numskip
+        dcd.setsread = dcd.setsread + numskip


Are you sure that this should now be one level out, i.e. not inside the if (step > 1 and skip >1) statement any more?

Yes.
Edit: Actually, I am not sure. Let me add some more tests.

Everything is fine, current tests account for this, the logic is sound too.

ok. Btw, can we write this aesthetically more pleasingly as

dcd.setsread += numskip

jdetle · 2016-07-31T18:36:34Z

The stop gymnastics has been done away with.

jdetle · 2016-07-31T19:22:49Z

@orbeckst Did a small squash hope that didn't delete your comments :/ Hopefully the new comments address your concerns.

coveralls · 2016-07-31T19:39:14Z

Coverage increased (+0.02%) to 83.13% when pulling d3d4dad on jdetle:slicing_fixes into df6cec8 on MDAnalysis:develop.

orbeckst · 2016-07-31T19:42:32Z

package/MDAnalysis/coordinates/dcdtimeseries.pyx

@@ -115,15 +115,21 @@ def __read_timecorrel(object self, object atoms, object atomcounts, object forma
    for i from 0 <= i < n_frames:
        if (step > 1 and i > 0):
            # Check if we have fixed atoms
-            # XXX not done


This comment said "We should check for fixed atoms here but we have not implemented it".

I don't think that you are addressing fixed atoms anywhere (it's an obscure feature in CHARMM DCD files where only the first frame contains the positions of the frozen atoms and all later frames do not have these positions, which are unchanging.) Therefore, please leave this comment in.

coveralls · 2016-07-31T19:51:21Z

Coverage decreased (-1.9%) to 81.196% when pulling d3d4dad on jdetle:slicing_fixes into df6cec8 on MDAnalysis:develop.

jdetle · 2016-07-31T20:07:42Z

Okay! I will fix it when I get back from my swim!
On Sun, Jul 31, 2016 at 12:51 PM Coveralls [email protected] wrote:

[image: Coverage Status] https://coveralls.io/builds/7241103

Coverage decreased (-3.3%) to 79.814% when pulling d3d4dad
d3d4dad
on jdetle:slicing_fixes into df6cec8
df6cec8
on MDAnalysis:develop.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#916 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AKcARlczxFAzKXAfSicTxKiNCZy_fCV2ks5qbPy6gaJpZM4JVv0L
.

Have a wonderful day,
John J. Detlefs

orbeckst · 2016-07-31T20:50:51Z

Hi John,

don't worry – it's ok to sometimes not watch the issue tracker ;-)… and I am told it's Sunday, too.

For me, hacking away feels like a reward and I really like doing it when I get a chance but there is really no expectation for anyone to be on call 24/7. Relaxed coders are good coders!

Oliver

On 31 Jul, 2016, at 13:07, John Detlefs [email protected] wrote:

Okay! I will fix it when I get back from my swim!

Oliver Beckstein * [email protected]
skype: orbeckst * [email protected]

richardjgowers · 2016-08-01T10:43:50Z

@jdetle give me a poke when this is ready to review

jdetle · 2016-08-01T15:04:28Z

Poke

richardjgowers · 2016-08-01T15:16:25Z

testsuite/MDAnalysisTests/coordinates/test_dcd.py

+    def test_timeseries_slicing(self):
+        # check that slicing behaves correctly
+        # should fail before issue #914 resolved
+        x = [(0, 1, 1), (1,1,1), (1, 2, 1), (1, 2, 2), (1, 4, 2), (1, 4, 4),


backwards stepping? (step = -1)

check that using None works too

Haha... annnnd its broken.

jdetle · 2016-08-01T16:56:11Z

So the functions for the DCD reader don't read in reverse from the best of my understanding and changing them to do this would probably be a pain. I don't know the best way of going about doing this, but it is certainly something we would want in the DCD Reader for python 3. Anyone have any thoughts?

richardjgowers · 2016-08-01T17:02:34Z

So has reverse iteration always been broken? (ie current develop state) if
so you can raise a new Issue and keep the failing tests as known failures
(might have to split into a new generator) and we can merge this as is.

On Mon, 1 Aug 2016, 17:56 John Detlefs, [email protected] wrote:

So the functions for the DCD reader don't read in reverse from the best of
my understanding and changing them to do this would probably be a pain. I
don't know the best way of going about doing this, but it is certainly
something we would want in the DCD Reader for python 3. Anyone have any
thoughts?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#916 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AI0jB_4WWFwVKyF6FNbxO3r7n7_ztY4qks5qbiUsgaJpZM4JVv0L
.

jdetle · 2016-08-01T17:09:20Z

Yes reverse iteration for the creation of a timeseries has been broken to begin with. For example, start = 5, stop =0 and step -1 returns a timeseries of array indices 5, 6, 7, 8, 9. I've got some work I have to get done on PCA but I'll raise an issue after I get things done on that. (Priorities!)

richardjgowers · 2016-08-01T17:12:18Z

Then it's arguably not your problem to fix in this PR. Make a separate
issue, add the failing tests as known failures in this PR.

On Mon, 1 Aug 2016, 18:09 John Detlefs, [email protected] wrote:

Yes reverse iteration for the creation of a timeseries has been broken to
begin with. Start = 5, stop =0 and step -1 returns a timeseries of array
indices 5, 6, 7, 8, 9.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#916 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AI0jB7oPdBCmusjY7UqBmJr8fVRpiRMAks5qbihBgaJpZM4JVv0L
.

coveralls · 2016-08-01T21:12:30Z

Coverage increased (+0.02%) to 84.158% when pulling 0f62787 on jdetle:slicing_fixes into 286e327 on MDAnalysis:develop.

kain88-de · 2016-08-02T08:41:23Z

The CHANGELOG doesn't mention that #818 is also fixed with this PR.

Slicing fixes

richardjgowers self-assigned this Jul 27, 2016

orbeckst reviewed Jul 27, 2016
View reviewed changes

jdetle force-pushed the slicing_fixes branch from 7343dd3 to 1c6c241 Compare July 27, 2016 23:59

orbeckst reviewed Jul 28, 2016
View reviewed changes

richardjgowers reviewed Jul 28, 2016
View reviewed changes

orbeckst mentioned this pull request Jul 28, 2016

DCDReader.timeseries(skip=XX) behaves differently from standard numpy slicing #818

Closed

orbeckst reviewed Jul 31, 2016
View reviewed changes

Added comments.

d3d4dad

jdetle force-pushed the slicing_fixes branch from 2f94dd5 to d3d4dad Compare July 31, 2016 19:21

orbeckst reviewed Jul 31, 2016
View reviewed changes

Undo comment removal [skip ci]

6833e3a

richardjgowers reviewed Aug 1, 2016
View reviewed changes

Added tests that cause failures

76ec586

jdetle force-pushed the slicing_fixes branch from 4634648 to 2f5f1ee Compare August 1, 2016 20:25

Added known failures

0f62787

jdetle force-pushed the slicing_fixes branch from 2f5f1ee to 0f62787 Compare August 1, 2016 20:45

richardjgowers merged commit a1aa844 into MDAnalysis:develop Aug 2, 2016

richardjgowers added this to the 0.16.0 milestone Aug 2, 2016

jdetle mentioned this pull request Aug 8, 2016

WIP: Dcd cython #929

Closed

12 tasks

orbeckst mentioned this pull request Sep 14, 2016

Trajectory slicing should be completely Pythonic #918

Closed

abiedermann pushed a commit to abiedermann/mdanalysis that referenced this pull request Oct 26, 2016

Merge pull request MDAnalysis#916 from jdetle/slicing_fixes

4bb20c6

Slicing fixes

Slicing fixes #916

Slicing fixes #916

Conversation

jdetle commented Jul 27, 2016 • edited Loading

Added known failures for backwards steps steps. PR Checklist

TODO

LINE CHANGES IN dcd.c

coveralls commented Jul 27, 2016 • edited Loading

jdetle commented Jul 27, 2016

richardjgowers commented Jul 27, 2016

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 Jul 27, 2016

jdetle commented Jul 27, 2016

jdetle commented Jul 28, 2016

coveralls commented Jul 28, 2016 • edited Loading

coveralls commented Jul 28, 2016 • edited Loading

orbeckst Jul 28, 2016 • edited Loading

Choose a reason for hiding this comment

orbeckst commented Jul 28, 2016

jdetle commented Jul 28, 2016

jdetle commented Jul 28, 2016

orbeckst commented Jul 28, 2016

jdetle commented Jul 28, 2016 • edited Loading

jdetle commented Jul 28, 2016

coveralls commented Jul 28, 2016 • edited Loading

Choose a reason for hiding this comment

orbeckst commented Jul 28, 2016

jdetle commented Jul 28, 2016 • edited Loading

coveralls commented Jul 28, 2016 • edited Loading

jdetle commented Jul 28, 2016

orbeckst Jul 31, 2016 • edited Loading

Choose a reason for hiding this comment

jdetle Jul 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdetle commented Jul 31, 2016

jdetle commented Jul 31, 2016

coveralls commented Jul 31, 2016 • edited Loading

Choose a reason for hiding this comment

coveralls commented Jul 31, 2016 • edited Loading

jdetle commented Jul 31, 2016

orbeckst commented Jul 31, 2016

richardjgowers commented Aug 1, 2016

jdetle commented Aug 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdetle commented Aug 1, 2016

richardjgowers commented Aug 1, 2016

jdetle commented Aug 1, 2016 • edited Loading

richardjgowers commented Aug 1, 2016

coveralls commented Aug 1, 2016 • edited Loading

kain88-de commented Aug 2, 2016

jdetle commented Jul 27, 2016 •

edited

Loading

Added known failures for backwards steps steps.
PR Checklist

coveralls commented Jul 27, 2016 •

edited

Loading

coveralls commented Jul 28, 2016 •

edited

Loading

coveralls commented Jul 28, 2016 •

edited

Loading

orbeckst Jul 28, 2016 •

edited

Loading

jdetle commented Jul 28, 2016 •

edited

Loading

coveralls commented Jul 28, 2016 •

edited

Loading

jdetle commented Jul 28, 2016 •

edited

Loading

coveralls commented Jul 28, 2016 •

edited

Loading

orbeckst Jul 31, 2016 •

edited

Loading

jdetle Jul 31, 2016 •

edited

Loading

coveralls commented Jul 31, 2016 •

edited

Loading

coveralls commented Jul 31, 2016 •

edited

Loading

jdetle commented Aug 1, 2016 •

edited

Loading

coveralls commented Aug 1, 2016 •

edited

Loading