Survival probability: Intermittency, Step, Residues, Overlapping SP Selection Example #2226

bieniekmateusz · 2019-03-21T16:02:19Z

Fixes #

"Step" parameter does not load frames which are not used (dependant on the value of tau_max). This is great of sampling long trajectories.

Changes made in this Pull Request:

residue-wise SP can be carried out
Intermittency is defined as a number of consecutive absences allowed
Intermittency combined with Step lead to consistent behaviour
the raw data are made available (helps with testing, allows building upon the values)
simplified and extended documentation to include SP around overlapping areas (for example, headgroups in lipids)
Test Cases for all new functionalities have been added

PR Checklist

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

We generalised the code to be applicable to other situations which is why we would like to ask if we should move it out of waterdynamics.py to its own file (survivalprobability.py in analysis).

The intermittency, for the sake of simplicity, is defined in terms of consecutive absences. This is calculated in a single pass over the data.

We've used the new functionalities and plan to publish on the topic to discuss the usefulness of these small changes.

This request is done on behalf of me and @p-j-smith

…nalysis. Before, all frames were loaded (and selection was always applied). Now, the frames which are not used are not loaded, thus improving the performance. This only happens when the step is larger than the tau_max + 1. Test cases cover the situation where some frames are skipped. The test checks how many times the 'select_atoms' was called. This is to help with the large size MD simulations analysis.

was set up or not. Also, manually moving between the frames rather then using "for _ in trajectory[]". This removed unnecessary variables.

border condition for "tau_max" and "step".

patch.

which atoms are found.

as a gap: meaning that setting it to the value of 2 means that the atom id 7, when in the sequence 7,X,X,7, where X means absence, will be rewritten to 7,7,7,7. This way the intermittency does not affect the actual routines for SP calculation. The array of IDs should never be big so this should not add any significant computational time.

to the selected IDs, using the selected IDs to verify the behaviour of the intermittency.

parameter is complex and was accounted for here, together with new test cases. If necessary, we load extra frames for each window, to account for the borders: for the first and last frame in a window.

analysis. This requires several runs and averaging.

codecov · 2019-03-21T16:34:15Z

Codecov Report

Merging #2226 into develop will decrease coverage by 1.12%.
The diff coverage is 95.74%.

@@             Coverage Diff             @@
##           develop    #2226      +/-   ##
===========================================
- Coverage    90.72%   89.59%   -1.13%     
===========================================
  Files           15      158     +143     
  Lines         1983    19725   +17742     
  Branches         0     2780    +2780     
===========================================
+ Hits          1799    17672   +15873     
- Misses         184     1458    +1274     
- Partials         0      595     +595

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/waterdynamics.py	`82.68% <95.74%> (ø)`
util.py	`88.14% <0%> (-0.02%)`	⬇️
package/MDAnalysis/coordinates/GRO.py	`93.87% <0%> (ø)`
package/MDAnalysis/coordinates/base.py	`93.3% <0%> (ø)`
package/MDAnalysis/analysis/helanal.py	`85.19% <0%> (ø)`
package/MDAnalysis/topology/MinimalParser.py	`100% <0%> (ø)`
package/MDAnalysis/visualization/__init__.py	`100% <0%> (ø)`
package/MDAnalysis/core/qcprot.py	`100% <0%> (ø)`
...e/MDAnalysis/analysis/encore/clustering/cluster.py	`96.22% <0%> (ø)`
package/MDAnalysis/coordinates/DCD.py	`97.16% <0%> (ø)`
... and 135 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1be9f73...657f918. Read the comment docs.

bieniekmateusz · 2019-03-22T12:28:33Z

Hi there, I only modified two files, however, I worked on the "develop" branch by mistake. So I tried to rebase the code to bring my commits to the front, which is why the review picks up so many files.

I'll see if I can move the commits differently to avoid what happened here.

bieniekmateusz · 2019-04-01T15:33:55Z

Hi again, I see that there are some functionalities which could help with organising the commits: git cherry-pick in particular. Let me know if it is necessary and then I'll further look into it.

orbeckst · 2019-04-03T02:09:47Z

Looks like a clean diff, so no need to play more with git rebase and friends.

…usz/mdanalysis into bieniekmateusz-survival_probability

orbeckst · 2019-04-03T02:11:43Z

I merged latest develop into your branch so that the docs build correctly and don't fail your build.

orbeckst

Primarily I have comments on the docs.

Also include an entry for CHANGELOG.

I would like to see a review by some of the original authors of SP, e.g., @alejob .

package/MDAnalysis/analysis/waterdynamics.py

orbeckst · 2019-04-03T02:36:00Z

We generalised the code to be applicable to other situations which is why we would like to ask if we should move it out of waterdynamics.py to its own file (survivalprobability.py in analysis).

I like the idea of moving it into its own module. How about doing this as a separate PR once this one is merged? The smaller a PR the better.

orbeckst · 2019-04-03T04:10:14Z

I broke travis #2233 so once PR #2234 is merged we need to again merge develop into this branch, sorry.

bieniekmateusz · 2019-04-03T14:12:08Z

The requested changes have been introduced. I've built the documentation and it appears fine. Cheers

package/MDAnalysis/analysis/waterdynamics.py

alejob · 2019-04-03T15:47:02Z

We generalised the code to be applicable to other situations which is why we would like to ask if we should move it out of waterdynamics.py to its own file (survivalprobability.py in analysis).

I like the idea of moving it into its own module. How about doing this as a separate PR once this one is merged? The smaller a PR the better.

It sounds good, but how could we avoid to broke the old code? There is people that has already implemented scripts with waterdynamics.survivalprobability(), so the idea is to avoid broke that code. Maybe some kind of link inside the MDA code.

bieniekmateusz · 2019-04-03T19:18:23Z

So moving it to another file would be a new PR in the future.

We could say that the code is moved and simply point the user to import the new path - if someone tries to use it. Luckily the function itself and how it works has not changed so it should be a minor inconvinience for the users, hopefully.

orbeckst · 2019-04-03T21:23:41Z

Breaking user code is not something that we do lightly. In this case it will be easy: we just import the function into water dynamics and everything else will just be able to use it as before. The call signature did not change (?) and all the old tests for the function still pass, don't they? In any case, that's all stuff we can discuss in a new issue/PR.

…

-- Oliver Beckstein email: orbeckst@gmail.com

Am Apr 3, 2019 um 15:18 schrieb Mateusz Bieniek ***@***.***>: So moving it to another file would be a new PR in the future. We could say that the code is moved and simply point the user to import the new path - if someone tries to use it. Luckily the function itself and how it works has not changed so it should be a minor inconvinience for the users, hopefully. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

…usz/mdanalysis into bieniekmateusz-survival_probability

orbeckst · 2019-04-04T20:15:17Z

Merged develop to get the travis fix from PR #2234

…alysis into survival_probability

bieniekmateusz · 2019-04-05T09:16:50Z

I think I've corrected everything, please let me know if I missed anything.

bieniekmateusz · 2019-04-05T13:52:15Z

To give you heads up, after this is merged we will shortly send you another PR with a common autocorrelation function between the waterdynamics.HydrogenBondLifetimes and waterdynamics.SurvivalProbability.

@richardjgowers noted that it would be good to formalise and extract the common autocorrelation code (PR #1995). After careful considerations with @p-j-smith we now have a separate autocorrelation function that has intermittency built into it, and being independent, is easily testable. The test cases will be well-defined in a similar manner to the ones we've used for the SurvivalProbability. We've already verified that waterdynamics.HydrogenBondLifetimes behaves exactly the same way our autocorrelation does (continuous). These changes should to a big extent simplify the module. In the case of HydrogenBondLifetimes it would mean a simple function call replacing the entire class. This will help simplifying the documentation too (PR #2181)

Work done with @p-j-smith.

orbeckst · 2019-04-05T17:37:00Z

package/CHANGELOG

@@ -15,7 +15,7 @@ The rules for this file:
 ------------------------------------------------------------------------------
 mm/dd/yy micaela-matta, xiki-tempula, zemanj, mattwthompson, orbeckst, aliehlen,
         dpadula85, jbarnoud, manuel.nuno.melo, richardjgowers, mattwthompson,
-         ayushsuhane, picocentauri, NinadBhat
+         ayushsuhane, picocentauri, NinadBhat, bieniekmateusz, p-j-smith


Are @bieniekmateusz and @p-j-smith in AUTHORS?

We, both of us. We're doing pair-programming.

orbeckst · 2019-04-05T17:41:45Z

@alejob if you're happy with the PR could you please also add a formal "approve" review? Thanks.

alejob

It looks a good extension to SP, and the new functionalities are clearly explained.

alejob · 2019-04-05T17:54:54Z

package/MDAnalysis/analysis/waterdynamics.py

+            leave a region of interest for up to two consecutive frames yet be treated as being present at all frames.
+            The default is continuous (0).
+        verbose : Boolean, optional
+            Print the progress to the console


I like this, it gives great flexibility to SP.

orbeckst · 2019-04-05T18:32:59Z

Thanks @alejob !

I resolved the conflict in the CHANGELOG and we'll just let CI confirm that everything is still good.

orbeckst · 2019-04-06T01:12:59Z

Congratulations @bieniekmateusz and @p-j-smith , merged! Thanks for your hard work (and patience).

Thanks @alejob for reviewing.

bieniekmateusz added 13 commits March 21, 2019 15:32

Use the same code for loading the frames regardless of whether the step

a0edf64

was set up or not. Also, manually moving between the frames rather then using "for _ in trajectory[]". This removed unnecessary variables.

A test case that checks if all frames are loaded. This is the

a9abc50

border condition for "tau_max" and "step".

Refactoring with the simpler "return_value" when possible with the mock

036140a

patch.

Better "verbose" message - avoid polluting.

f45a005

The SP value of entire residues can be calculated, regardless

91037bc

which atoms are found.

Refactored intermittency: fewer nested blocks, giving the user access

91a1781

to the selected IDs, using the selected IDs to verify the behaviour of the intermittency.

Minor updates to the documentation.

4dde01f

New test cases for the step+intermittency relation. - not finished

ba79bcf

The relationship between the intermittency and the window "step"

ff4d266

parameter is complex and was accounted for here, together with new test cases. If necessary, we load extra frames for each window, to account for the borders: for the first and last frame in a window.

Corrected documentation / proofreading.

417d229

An example covering the case with multiple references for the SP

3576984

analysis. This requires several runs and averaging.

orbeckst added the Component-Analysis label Apr 3, 2019

Merge branch 'survival_probability' of https://github.com/bieniekmate…

4083378

…usz/mdanalysis into bieniekmateusz-survival_probability

orbeckst requested changes Apr 3, 2019

View reviewed changes

orbeckst mentioned this pull request Apr 3, 2019

fix travis file (tab -> space) #2234

Merged

4 tasks

bieniekmateusz added 2 commits April 3, 2019 11:39

Logging changes made regarding Survival Probability

b67a7da

Corrections from PR MDAnalysis#2226

970e05b

alejob reviewed Apr 3, 2019

View reviewed changes

package/MDAnalysis/analysis/waterdynamics.py Outdated Show resolved Hide resolved

Merge branch 'survival_probability' of https://github.com/bieniekmate…

3a46b34

…usz/mdanalysis into bieniekmateusz-survival_probability

bieniekmateusz added 2 commits April 5, 2019 10:14

Numpy documentation style: PR MDAnalysis#2226

aaf30bc

Merge branch 'survival_probability' of github.com:bieniekmateusz/mdan…

d2b4f08

…alysis into survival_probability

orbeckst reviewed Apr 5, 2019

View reviewed changes

orbeckst approved these changes Apr 5, 2019

View reviewed changes

alejob approved these changes Apr 5, 2019

View reviewed changes

Merge branch 'develop' into survival_probability

657f918

orbeckst self-assigned this Apr 5, 2019

p-j-smith mentioned this pull request Apr 5, 2019

Hbond analysis #2237

Merged

4 tasks

orbeckst merged commit 668e183 into MDAnalysis:develop Apr 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Survival probability: Intermittency, Step, Residues, Overlapping SP Selection Example #2226

Survival probability: Intermittency, Step, Residues, Overlapping SP Selection Example #2226

bieniekmateusz commented Mar 21, 2019 •

edited by orbeckst

codecov bot commented Mar 21, 2019 •

edited

bieniekmateusz commented Mar 22, 2019

bieniekmateusz commented Apr 1, 2019

orbeckst commented Apr 3, 2019

orbeckst commented Apr 3, 2019

orbeckst left a comment

orbeckst commented Apr 3, 2019

orbeckst commented Apr 3, 2019

bieniekmateusz commented Apr 3, 2019

alejob commented Apr 3, 2019

bieniekmateusz commented Apr 3, 2019

orbeckst commented Apr 3, 2019 via email

orbeckst commented Apr 4, 2019

bieniekmateusz commented Apr 5, 2019

bieniekmateusz commented Apr 5, 2019

orbeckst Apr 5, 2019

bieniekmateusz Apr 5, 2019

orbeckst commented Apr 5, 2019

alejob left a comment

alejob Apr 5, 2019

orbeckst commented Apr 5, 2019

orbeckst commented Apr 6, 2019

Survival probability: Intermittency, Step, Residues, Overlapping SP Selection Example #2226

Survival probability: Intermittency, Step, Residues, Overlapping SP Selection Example #2226

Conversation

bieniekmateusz commented Mar 21, 2019 • edited by orbeckst

PR Checklist

codecov bot commented Mar 21, 2019 • edited

Codecov Report

bieniekmateusz commented Mar 22, 2019

bieniekmateusz commented Apr 1, 2019

orbeckst commented Apr 3, 2019

orbeckst commented Apr 3, 2019

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst commented Apr 3, 2019

orbeckst commented Apr 3, 2019

bieniekmateusz commented Apr 3, 2019

alejob commented Apr 3, 2019

bieniekmateusz commented Apr 3, 2019

orbeckst commented Apr 3, 2019 via email

orbeckst commented Apr 4, 2019

bieniekmateusz commented Apr 5, 2019

bieniekmateusz commented Apr 5, 2019

orbeckst Apr 5, 2019

Choose a reason for hiding this comment

bieniekmateusz Apr 5, 2019

Choose a reason for hiding this comment

orbeckst commented Apr 5, 2019

alejob left a comment

Choose a reason for hiding this comment

alejob Apr 5, 2019

Choose a reason for hiding this comment

orbeckst commented Apr 5, 2019

orbeckst commented Apr 6, 2019

bieniekmateusz commented Mar 21, 2019 •

edited by orbeckst

codecov bot commented Mar 21, 2019 •

edited