Nongaussian parameter #3896
Nongaussian parameter #3896
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3896 +/- ##
===========================================
- Coverage 93.60% 93.59% -0.01%
===========================================
Files 171 183 +12
Lines 21235 22316 +1081
Branches 3933 3938 +5
===========================================
+ Hits 19876 20886 +1010
- Misses 899 971 +72
+ Partials 460 459 -1 ☔ View full report in Codecov by Sentry. |
|
@jaclark5, very exciting. I'll need to read the paper and understand the equations first. I'll try and get to it by the end of the week. |
| @@ -267,6 +268,10 @@ | |||
| class EinsteinMSD(AnalysisBase): | |||
| r"""Class to calculate Mean Squared Displacement by the Einstein relation. | |||
|
|
|||
| If `fft=False` so that the "windowed" algorithm is used, the second order | |||
There was a problem hiding this comment.
I think we need to make the parameter an optional addition to the fft=False run. MSDs are already expensive (esp without an FFT) and tacking on more compulsory computation is not ideal. Much better to have it as an optional kwarg.
There was a problem hiding this comment.
Fair enough, that also begs the question on whether this package will move toward cython modules?
There was a problem hiding this comment.
for the MSD package? There is likely some benefit there. We are generally encouraging of Cython contributions and have a bunch of extensions already written in Cython.
If you go down the cython side and need a hand feel free to ping. See setup.py for the minimal setup for a cython extension.
| A selection string. Defaults to "all" in which case | ||
| all atoms are selected. | ||
| msd_type : {'xyz', 'xy', 'yz', 'xz', 'x', 'y', 'z'} | ||
| msd_type : {'xyz', 'xy', 'yz', 'xz', 'x', 'y', 'z'} (optional) |
There was a problem hiding this comment.
Nitpick, idk what other people would say but personally I wouldn't say optional because has a default and will raise exception otherwise.
There was a problem hiding this comment.
I actually changed the doc string to include the dtype instead of the allowed options. As for the use of optional, let me know what you want, but I thought the fact that it's a keyword argument with a default is what makes this an optional input.
There was a problem hiding this comment.
JK I realized that listing the options is a convention in MDAnalysis
jaclark5
force-pushed
the
nongaussian
branch
from
30116ca to
9d33765
Compare
|
Sorry @jaclark5 I'll try and get to this ASAP. I just need to read some theory first. 😅 |
|
Really don't worry about it, I downgraded this PR to a draft to look into use of cython. I have three other PRs that are closer to being done and I think more impactful: |
Conflicts: package/CHANGELOG package/doc/sphinx/source/references.bib
|
@jaclark5, lets not let this fantastic PR go stale, was there any more you were planning to add or should I review? |
|
@jaclark5 bump, there is also the option to move this to the |
Conflicts: package/CHANGELOG package/MDAnalysis/analysis/msd.py
|
Hello @jaclark5! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-07-02 12:50:20 UTC |
Linter Bot Results:Hi @jaclark5! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
|
@hmacdope can I please assign you to manage the PR? If this doesn't work for you please unassign yourself and let me know. Thanks! |
|
@jaclark5 are you still interested in pursuing this PR? |
|
@hmacdope, it's ready to go here and I'm open to moving forward with it. However, isn't it the main developers' preference that it be incorporated into an MDAkit? |
|
@jaclark5 I think the way forward here will be to migrate this to |
|
That's fine. I did take a quick look at that and it's rougher than I thought. The existing MSD functionality isn't there yet. Is there a plan to have the existing msd tool contents moved? |
Fixes #3895
Changes made in this Pull Request:
Tests were added to existing msd tests, but here a comparison of TIP4P2005 water at 300K is compared to the published result [DOI: 10.1073/pnas.1900239116].
PR Checklist