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

Add in analysis function to calculate MSDs #2619

Merged
merged 136 commits into from
Jun 15, 2020

Conversation

hmacdope
Copy link
Member

@hmacdope hmacdope commented Mar 13, 2020

Fixes #2438

This is a WIP implementation for the calculation of Mean Squared Displacements

Changes made in this Pull Request:

  • Add in experimental code for calculation of MSD
  • both simple "windowed" calculation (N^2)
  • more sophisticated FFT based calculation (Nlog(N))
  • some tests

This needs a fair bit more work, including more tests and docs

PR Checklist

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

@hmacdope hmacdope marked this pull request as ready for review March 13, 2020 02:06
@hmacdope hmacdope changed the title Add msd Add in analysis function to calculate MSDs Mar 13, 2020
@codecov
Copy link

codecov bot commented Mar 13, 2020

Codecov Report

Merging #2619 into develop will decrease coverage by 0.00%.
The diff coverage is 92.85%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2619      +/-   ##
===========================================
- Coverage    91.17%   91.17%   -0.01%     
===========================================
  Files          176      177       +1     
  Lines        23798    23787      -11     
  Branches      3134     3137       +3     
===========================================
- Hits         21699    21687      -12     
- Misses        1476     1478       +2     
+ Partials       623      622       -1     
Impacted Files Coverage Δ
package/MDAnalysis/analysis/msd.py 92.85% <92.85%> (ø)
auxiliary/base.py 90.84% <0.00%> (-0.56%) ⬇️
coordinates/base.py 94.42% <0.00%> (-0.30%) ⬇️
util.py 88.04% <0.00%> (-0.09%) ⬇️
package/MDAnalysis/lib/transformations.py 78.62% <0.00%> (+0.43%) ⬆️

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 085ac0f...16dca64. Read the comment docs.

@richardjgowers
Copy link
Member

@hmacdope this is cool, MSD is a frequent request. Have you looked at this package yet? https://github.com/pdebuyl-lab/tidynamics

Iirc the package is well implemented, so it might be a good idea to make your MSD tool just a wrapped that squeezes MDA objects into tidynamics.

@hmacdope
Copy link
Member Author

Hey Richard, i'll have a look for sure!
What I have written so far covers the FCA algorithm (autocorrFFT) but they have several functions to do with cross correlations that I am missing.

I would be happy to wrap around the library, however a quick look indicates to me that it seems to deal with single particle data (2D ndarrays), ie we would need repeated calls to their MSD function for each particle, operating on some kind of slice of our 3D ndarray trajectory (time,particle,xyz). Im unsure about the overhead for this, but it can be avoided with a bit of refactoring, which is what I have tried to do.
Sorry for the long comment!
Would love to hear any thoughts

@hmacdope
Copy link
Member Author

also think could benefit from some cythonization

@hmacdope
Copy link
Member Author

@lilyminium is there a bilayer trajectory in the test set? This could be useful for demo purposes. I have a few lying around, but understand the desire to keep everything lightweight.

@lilyminium
Copy link
Member

@hmacdope GRO_MEMPROT, XTC_MEMPROT. A full list of datafiles and a description of them is in testsuite/MDAnalysisTests/datafiles.py.

@hmacdope
Copy link
Member Author

Thanks so much that makes my life so much easier.

@richardjgowers
Copy link
Member

@hmacdope this is the sort of thing that might need a bigger file than we comfortably can fit in the testsuite. In which case maybe a mini regression test (ie checking you get a precalculated number again https://github.com/MDAnalysis/mdanalysis/pull/2622/files#diff-9b2087bc18e88659747785690262c888R46-R51) against the file linked above is fine. But then in addition there'll need to be some sort of larger test against g_msd or similar to check it's sane.

@hmacdope
Copy link
Member Author

@richardjgowers thanks. I'll come up with some good regression tests.
Also are you happy if I add tidynamics as a dependency?
I am having some trouble with the accuracy of larger FFTs, The tidynamics implementation is efficient, and seems to dodge this problem.

@hmacdope
Copy link
Member Author

Some ideas for low cost regression tests include toy problems, ie the unit step trajectory which fits y = Dx^2 exactly.

@hmacdope
Copy link
Member Author

hmacdope commented Mar 14, 2020

Sorry for the comment spam, it appears that FFT based algorithms (tidynamics included) are only accurate for large toy MSDs (10000+ frames) out to about the fourth dp. This was causing me some confusion. Ill keep you posted on regression tests.

@orbeckst
Copy link
Member

orbeckst commented Jun 13, 2020

@hmacdope please look at your two failures: one is messed up docs in EinsteinMSD, the other is a missing Py 2 construct: for the latter, merge develop, which removes the check for Py 2 things. (EDIT: and don’t keep Py 2 constructs in the PR... Py 2 is gone).

Once the tests pass, everything should be good to go (finally!).

@hmacdope
Copy link
Member Author

I think we should be okay now? Any qualms @lilyminium, @richardjgowers?

@orbeckst
Copy link
Member

@lilyminium @richardjgowers please make sure that you that when you're happy you squash and merge as opposed to merging these 136 commits... thanks. (Also thanks from ASV, which prefers to run on fewer commits and also likes having merge commits as opposed to rebases.)

@lilyminium
Copy link
Member

@hmacdope are you interested in editing your commit history into a handful of commits or would you prefer this PR get squashed into 1? I did a similar thing with the hole2 module (#2523)

@orbeckst I didn't know that ASV... thanks, will consider in the future

@hmacdope
Copy link
Member Author

@lilyminium difficult to collect commits that would stand on their own I think. Just squash it.

Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @hmacdope!

@lilyminium lilyminium merged commit 46e505c into MDAnalysis:develop Jun 15, 2020
@hmacdope
Copy link
Member Author

Thanks to everyone involved, was a really fun experience. :)

@orbeckst
Copy link
Member

Congratulations on getting this PR finally merged. 253 comments (so far) is a high number, even by our standards. Thanks for your patience and seeing it through until the end.

PicoCentauri added a commit to PicoCentauri/mdanalysis that referenced this pull request Mar 30, 2021
 - Add in experimental code for calculation of MSD
 - both simple "windowed" calculation  (N^2)
 - more sophisticated FFT based calculation (Nlog(N))
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.

Generalize the water MSD calculation function to any molecule.
9 participants