Feature non linear time averaged msd #5066

gitsirsha · 2025-06-15T18:16:34Z

Changes made in this Pull Request:

msd.py script was modified to support time-averaged mean square displacement calculation for dump files with non-linear time spacings.

PR Checklist

Issue raised/referenced?
Yes Issue Support for MDAnalysis packages dealing with non-linear time dump #5028
Tests updated/added?
Not Yet
Documentation updated/added?
Updated short documentation at the top of the msd.py as comments
package/CHANGELOG file updated?
Not Yet
Is your name in package/AUTHORS? (If it is not, add it!)
Yes - Added

Developers Certificate of Origin

I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.

📚 Documentation preview 📚: https://mdanalysis--5066.org.readthedocs.build/en/5066/

…ar dump

github-actions

Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on GitHub Discussions so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.

codecov · 2025-06-15T18:30:58Z

Codecov Report

Attention: Patch coverage is 94.28571% with 2 lines in your changes missing coverage. Please review.

Project coverage is 86.14%. Comparing base (0dc5a1b) to head (f6e5ddf).

Files with missing lines	Patch %	Lines
package/MDAnalysis/analysis/msd.py	94.28%	1 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (0dc5a1b) and HEAD (f6e5ddf). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (0dc5a1b) HEAD (f6e5ddf)

2 1

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5066      +/-   ##
===========================================
- Coverage    93.62%   86.14%   -7.49%     
===========================================
  Files          177      177              
  Lines        22001    22036      +35     
  Branches      3114     3119       +5     
===========================================
- Hits         20599    18982    -1617     
- Misses         947     2604    +1657     
+ Partials       455      450       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mrshirts

Added some comments on the code.

mrshirts · 2025-06-18T17:49:49Z

package/MDAnalysis/analysis/msd.py

+#   Modified by Sirsha Ganguly
+#
+# Modification:
+# '_conclude_modified()' is the new function added to calculate time-averaged mean squared displacement of non-linear dumps


Suggest something more descriptive here - _conclude_nonlinear() instead of _conclude_modified()

mrshirts · 2025-06-18T17:59:12Z

package/MDAnalysis/analysis/msd.py

+        if len(sequence) < 2:
+            return True
+        step = sequence[1] - sequence[0]
+        return all(sequence[i+1] - sequence[i] == step for i in range(len(sequence) - 1))


So right now, it's failing the test. My guess is that it's going down the wrong test path, because it's interpreting whatever data is being passed in by the test as not actually being linear (maybe something like 1.0000 gets interpreted at 0.999999). A couple of options here:

Add a keyword like nonlinear that controls the logic, rather than determining the logic from the steps.

Using something like assert_almost_equal so floating point errors don't cause the logic to give the wrong answer (i.e. that they are not equally spaced when they actually are).

IAlibay

Here are some initial questions before we get into the weeds. I'm keen for us to end up with a single method that works for both use cases if possible.

There are probably things we can do to make that happen, but I first went to get a good idea of where things are at in this current state.

IAlibay · 2025-06-19T19:09:20Z

package/MDAnalysis/analysis/msd.py

@@ -1,3 +1,25 @@
+######## Documentation


It's fine to do this later once code has been settled on, but documents exist as module docstring, or API docstring (see the sections encompassed by """ below).

As Irfan said: The comments will not be in the final PR. The first line of the file is always

# -*- Mode: python; tab-width: 4; indent-tabs-mode:nil; coding:utf-8 -*-

and then comes the standard comment block. The only history information (eg who modified the file or added something) that we keep inside the code is versionchanged/versionadded markup in the doc strings. Everything else is known to be available through the git history and the information in the PR/issue.

Could you please

remove the comments

copy them to the PR information

This will make it a lot easier to discuss the intention of the PR separate from the implementation.

Information on the algorithm (basically what's important for a user to know) will go into the doc string in a Notes section (we follow the numpy doc string standard and see https://userguide.mdanalysis.org/stable/contributing_code.html#guidelines-for-docstrings for MDAnalysis specific guidelines on docs).

IAlibay · 2025-06-19T19:18:54Z

package/MDAnalysis/analysis/msd.py

@@ -1,3 +1,25 @@
+######## Documentation
+#
+# Issue: The default '_conclude_simple()' function previously calculated time-averaged mean squared displacement considering linear timespacing between the frames even when a dump file with non-linear timestep gap is provided


It would be better if these details were in the PR info instead - this is where a reviewer usually looks for them (and ultimately we will likely merge without this).

IAlibay · 2025-06-19T19:22:21Z

package/MDAnalysis/analysis/msd.py

+# Modification:
+# '_conclude_modified()' is the new function added to calculate time-averaged mean squared displacement of non-linear dumps
+    # Note: This new function is generalized for non-linear dumps.
+    #       Moreover, if you use this function to calculate for linear timedump this gives same result as '_conclude_simple()' which is implemented in the default version.


Thanks for checking this, it's really what we need to discuss the most here. Ultimately I think the end target is this PR will be to end up with just one method. Having alternate code paths adds to user difficulty and maintenance costs.

Just taking things as they are now (before we get into the weeds).Could you comment on performance? Do the two methods run at similar speeds with the same inputs?

What happens if someone uses the fft based method instead with non linear times? Does that fail?

orbeckst

Thank you for the PR. That's a good start for a discussion.

Initially it would really help to have the big picture up front, namely a description at a high level what you want to accomplish and how.

Additionally

Describe the algorithm that you're implementing.
How does it differ from the existing one (without FFT)?
Can you provide a rough performance comparison between "linear" and "nonlinear"? (I understand that applying the algorithm for equally spaced frames to a trajectory with variably spaced frames is incorrect but I want to get a sense of the necessary added computational complexity.)

I added other comments throughout but initially the big picture is more important because we are trying to figure out if there are ways to re-use code: the fewer code is duplicated, the lower the maintenance burden that we incur, and the easier it is to test and maintain correctness.

orbeckst · 2025-06-26T22:13:12Z

package/MDAnalysis/analysis/msd.py

@@ -1,3 +1,25 @@
+######## Documentation


As Irfan said: The comments will not be in the final PR. The first line of the file is always

# -*- Mode: python; tab-width: 4; indent-tabs-mode:nil; coding:utf-8 -*-

and then comes the standard comment block. The only history information (eg who modified the file or added something) that we keep inside the code is versionchanged/versionadded markup in the doc strings. Everything else is known to be available through the git history and the information in the PR/issue.

Could you please

remove the comments

copy them to the PR information

This will make it a lot easier to discuss the intention of the PR separate from the implementation.

Information on the algorithm (basically what's important for a user to know) will go into the doc string in a Notes section (we follow the numpy doc string standard and see https://userguide.mdanalysis.org/stable/contributing_code.html#guidelines-for-docstrings for MDAnalysis specific guidelines on docs).

orbeckst · 2025-06-26T22:14:05Z

package/MDAnalysis/analysis/msd.py

 class EinsteinMSD(AnalysisBase):
+
+    @staticmethod
+    def is_linear(sequence): # This function is to check if the timesteps in the array is linear or not! If it's linear default MDA code will carry-out other wise the modified code carries!


Instead of comments, write doc strings.

orbeckst · 2025-06-26T22:16:13Z

package/MDAnalysis/analysis/msd.py

+
+
+    def _conclude_modified(self):
+        from collections import defaultdict


imports at top of file,

import collections

then use collections.defaultdict

orbeckst · 2025-06-26T22:16:50Z

package/MDAnalysis/analysis/msd.py

+
+    def _conclude_modified(self):
+        from collections import defaultdict
+        print("The Dump file has non-linear time spacing")


Eventually, remove all print – if you want to show status messages, use the logger.

orbeckst · 2025-06-26T22:17:50Z

package/MDAnalysis/analysis/msd.py

+        from collections import defaultdict
+        print("The Dump file has non-linear time spacing")
+
+        dump_times = [ts.time for ts in self._trajectory]


This iterates through the whole trajectory and may already be expensive!

Maybe unavoidable but keep it in mind.

orbeckst · 2025-06-26T22:18:47Z

package/MDAnalysis/analysis/msd.py

+        n_frames = len(dump_times)
+        n_atoms = self.n_particles
+        positions = np.zeros((n_frames, n_atoms, 3))
+        positions = self._position_array.astype(np.float64)


This overwrites positions. Delete the line above?? Not sure what the intent was here.

gitsirsha added 3 commits June 13, 2025 17:43

Inditial addition of code to calculate time-averaged msd for non-line…

045f91f

…ar dump

updated_documentation_v1_plots_removed

3462365

Added_name_in_the_package/AUTHORS

f6e5ddf

github-actions bot reviewed Jun 15, 2025

View reviewed changes

mrshirts reviewed Jun 18, 2025

View reviewed changes

IAlibay reviewed Jun 19, 2025

View reviewed changes

orbeckst requested changes Jun 26, 2025

View reviewed changes



		def _conclude_modified(self):
		from collections import defaultdict

Feature non linear time averaged msd #5066

Are you sure you want to change the base?

Feature non linear time averaged msd #5066

Conversation

gitsirsha commented Jun 15, 2025 • edited by orbeckst Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Developers Certificate of Origin

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 15, 2025

Codecov Report

Uh oh!

mrshirts left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gitsirsha commented Jun 15, 2025 •

edited by orbeckst

Loading