Restructured WaterOrientationalRelaxation #3247

PicoCentauri · 2021-04-28T16:12:54Z

Partly Fixes MDAnalysis/waterdynamics#22

Changes made in this Pull Request:

The WaterOrientationalRelaxation was restructured to follow the start, stop and step attributes of the AnalysisBase.
The documentation was also improved and the example was adjusted to a pure water system. I also moved everything
into the class documentation.

It seems that the results after my restructuring are okay but maybe somebody who knows how these correlation function should look takes a look at the results.

PR Checklist

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

pep8speaks · 2021-04-28T16:12:57Z

Hello @PicoCentauri! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file package/MDAnalysis/analysis/waterdynamics.py:

Line 376:80: E501 line too long (82 > 79 characters)
Line 430:78: W291 trailing whitespace

Comment last updated at 2021-04-30 21:06:00 UTC

codecov · 2021-04-28T16:32:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.84%. Comparing base (f2745a9) to head (bf39ed5).
Report is 1155 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3247      +/-   ##
===========================================
+ Coverage    92.81%   92.84%   +0.03%     
===========================================
  Files          170      170              
  Lines        22801    22810       +9     
  Branches      3239     3241       +2     
===========================================
+ Hits         21162    21179      +17     
+ Misses        1591     1583       -8     
  Partials        48       48

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

tylerjereddy

I added a few drive-by copy edit and test suggestions, but maybe you want to avoid adding more changes to keep the diff small for now.

package/MDAnalysis/analysis/waterdynamics.py

testsuite/MDAnalysisTests/analysis/test_waterdynamics.py

orbeckst · 2021-05-06T00:43:14Z

Are these changes are backwards-compatible?

PicoCentauri · 2021-05-07T07:19:11Z

In some way:

t0, the argument for the initial step was silently ignored.
dtmax, was the argument for the maximal step size considered for the correlation function. During my tests it seemed that only dtmax=2worked. Therefore, I removed the argument dtmax.

orbeckst · 2021-05-07T23:23:26Z

It sounds as if these changes get an entry under "Fixes" in the CHANGELOG. Then this PR can go in anytime after 2.0.0.

orbeckst · 2021-05-07T23:24:10Z

@alejob could you please review this PR? Thanks!

alejob · 2021-05-10T23:20:50Z

In some way:

t0, the argument for the initial step was silently ignored.

dtmax, was the argument for the maximal step size considered for the correlation function. During my tests it seemed that only dtmax=2worked. Therefore, I removed the argument dtmax.

Hi there!

dtmax is an important part of WOR analysis, this because is the time windows where the water molecules relax. Did this change pass the unitary test?

edit: Ok, I see that it has a new name.

orbeckst

I am not sure yet if dtmax is the same as step — @alejob I'd be grateful for a quick explanation (see comments).

We did not announce/deprecate these changes in 1.x so we have to provide ways to use the old usage during 2.x.

orbeckst · 2021-05-11T18:24:08Z

package/MDAnalysis/analysis/waterdynamics.py

+    .. versionchanged:: 2.0.0
+       `t0`, `tf` and `dtmax` are renamed to start, stop, step as attributes
+       of the `run` method providing a consistent behaviour with other
+       modules.


We did not deprecate t0, tf, dtmax in 1.1.1 — they are still all documented as standard

mdanalysis/package/MDAnalysis/analysis/waterdynamics.py

Lines 685 to 696 in 1e456e8

frame where analysis begins

tf : int

frame where analysis ends

dtmax : int

Maximum dt size, `dtmax` < `tf` or it will crash.

.. versionadded:: 0.11.0

.. versionchanged:: 1.0.0

Changed `selection` keyword to `select`

"""

This means that for 2.x, it still needs to be possible to use the old code. But you can make them optional kwargs

def __init__(self, universe, select, t0=None, tf=None, dtmax=None):

If they are not None, issue a deprecation warning. Fail with TypeError("If the deprecated t0, tf, and dtmax are used, all of them are required") if not all three of them are set (as the original code would have done).

You might have to subclass run() to substitute t0, tf, dtmax for start, stop, step.

If run() replaces start/stop/step then issue a deprecation warning, too.

orbeckst · 2021-05-11T18:26:01Z

package/MDAnalysis/analysis/waterdynamics.py

    """

-    def __init__(self, universe, select, t0, tf, dtmax, nproc=1):


Document removal of nproc as a versionchanged ("Removed optional nproc kwarg that was not used.") and in CHANGELOG Changes.

I think we can get away without deprecations for this one.

orbeckst · 2021-05-11T18:28:41Z

package/MDAnalysis/analysis/waterdynamics.py

+
+    def _get_repeated_indices(self, selection, frame):
+        """
+        Indicates the comparasion between the current frame and all


orbeckst · 2021-05-11T18:32:21Z

package/MDAnalysis/analysis/waterdynamics.py

-    a unit vector along HH, OH or dipole vector.
-
+    where :math:`P_2=(3x^2-1)/2` is the second-order Legendre polynomial
+    and :math:`\hat{u}` is a unit vector along HH, OH or dipole vector.


Please add a warning that this class assumes a 3-point water model where each water residue contains O as the first entry and hydrogen 1 and 2 as second and third entry.

This warning should have been here from the beginning. Even better would be a more robust approach to choosing O and H1, H2 but that's not the job of this PR.

orbeckst · 2021-05-11T18:34:39Z

package/MDAnalysis/analysis/waterdynamics.py


-        for j in range(totalFrames // dt - 1):
-            a = self._getOneDeltaPoint(universe, repInd, j, sumsdt, dt)
+        for i in range(self.start, self.stop // self.frame - 1):


I don't understand why self.stop // self.frame - 1. Can you add a comment here explaining what this loop does?

orbeckst · 2021-05-11T18:37:32Z

package/MDAnalysis/analysis/waterdynamics.py

-    dtmax : int
-      Maximum dt size, `dtmax` < `tf` or it will crash.


@alejob what is dtmax supposed to be doing?

Is it the same as a stride when you subselect frames from a trajectory as in trajectory[start:stop:step]? If not, please explain.

It is not the same as trajectory[start:stop:step].

First, we need to define dt. dt is the delta time in which WOR is calculated. For instance, if dt=5 this means that WOR will be calculated comparing frame 1 with frame 5, and then comparing frame 6 with 10, then 11 with 15, and so on. Then we get the mean of these values and get the final value for dt=5. If dt=8, WOR will be calculated between frame 1 and frame 8, between 9 and 16 and repeat all the process. Now, dtmax indicates the maximum value that dt could have, and indicates that all the minor dt will be calculated, in other words, if you plot the output of this analysis, dtmax is the maximum value of the x axis.

Here an example modified from the docs:
If you have that t0 = 0, tf = 1000 and dtmax = 20, we create 20 windows
timesteps (20 values in the x axis), the first window is created with 1000
timestep average (1000/1), the second window is created with 500 timestep
average(1000/2), the third window is created with 333 timestep average (1000/3)
and so on. Obtaining the mean of many timesteps gives us a better statistic for the WOR value, i.e, a smoother curve.

Because of this to fix dtmax in dtmax=2 has non sense, dtmax should be chosen by the user.

orbeckst · 2022-04-14T08:42:37Z

@PicoCentauri is this PR something you might want to come back to?

PicoCentauri · 2022-04-14T15:41:34Z

Currently, I have to no time to look into this in great details. When we had the last round of nice feedback I had the feeling that the current state of the class is physically broken and that a transition to the AnalysisBase is not as straightforward as I thought in the beginning.

IAlibay · 2022-05-16T12:56:38Z

Currently, I have to no time to look into this in great details. When we had the last round of nice feedback I had the feeling that the current state of the class is physically broken and that a transition to the AnalysisBase is not as straightforward as I thought in the beginning.

@PicoCentauri this is on the 2.2.0 milestone, from this comment shall I assume that this is not going to make it? (I've not been keeping up with the review comments sorry).

If so, which future milestone shall I add this to? 2.3.0 or 3.0.0?

PicoCentauri · 2022-05-17T08:07:05Z

Probabaly 3.0.0

Restructured WaterOrientationalRelaxation

a0c9c4f

Fixed PEP8 issues

a104835

PicoCentauri force-pushed the 2719-waterdynamics-to-AnalysisBase branch from 417e20e to a104835 Compare April 28, 2021 16:23

tylerjereddy added the Component-Analysis label Apr 29, 2021

tylerjereddy reviewed Apr 30, 2021

View reviewed changes

Applied suggestions

bf39ed5

IAlibay requested a review from alejob May 2, 2021 11:15

orbeckst added this to the 2.1.0 milestone May 7, 2021

orbeckst requested changes May 11, 2021

View reviewed changes

orbeckst modified the milestones: 2.1.0, 2.2.0 Apr 14, 2022

IAlibay modified the milestones: 2.2.0, 3.0 Jun 1, 2022

PicoCentauri closed this Jun 5, 2024

PicoCentauri deleted the 2719-waterdynamics-to-AnalysisBase branch June 5, 2024 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructured WaterOrientationalRelaxation #3247

Restructured WaterOrientationalRelaxation #3247

PicoCentauri commented Apr 28, 2021

pep8speaks commented Apr 28, 2021 •

edited

Loading

codecov bot commented Apr 28, 2021 •

edited

Loading

tylerjereddy left a comment

orbeckst commented May 6, 2021

PicoCentauri commented May 7, 2021

orbeckst commented May 7, 2021

orbeckst commented May 7, 2021

alejob commented May 10, 2021 •

edited

Loading

orbeckst left a comment

orbeckst May 11, 2021

orbeckst May 11, 2021

orbeckst May 11, 2021

orbeckst May 11, 2021

orbeckst May 11, 2021

orbeckst May 11, 2021

orbeckst May 11, 2021

alejob May 14, 2021 •

edited

Loading

orbeckst commented Apr 14, 2022

PicoCentauri commented Apr 14, 2022 •

edited

Loading

IAlibay commented May 16, 2022

PicoCentauri commented May 17, 2022

	frame where analysis begins
	tf : int
	frame where analysis ends
	dtmax : int
	Maximum dt size, `dtmax` < `tf` or it will crash.


	.. versionadded:: 0.11.0

	.. versionchanged:: 1.0.0
	Changed `selection` keyword to `select`
	"""

		"""

		def __init__(self, universe, select, t0, tf, dtmax, nproc=1):

Restructured WaterOrientationalRelaxation #3247

Restructured WaterOrientationalRelaxation #3247

Conversation

PicoCentauri commented Apr 28, 2021

PR Checklist

pep8speaks commented Apr 28, 2021 • edited Loading

Comment last updated at 2021-04-30 21:06:00 UTC

codecov bot commented Apr 28, 2021 • edited Loading

Codecov Report

tylerjereddy left a comment

Choose a reason for hiding this comment

orbeckst commented May 6, 2021

PicoCentauri commented May 7, 2021

orbeckst commented May 7, 2021

orbeckst commented May 7, 2021

alejob commented May 10, 2021 • edited Loading

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst May 11, 2021

Choose a reason for hiding this comment

orbeckst May 11, 2021

Choose a reason for hiding this comment

orbeckst May 11, 2021

Choose a reason for hiding this comment

orbeckst May 11, 2021

Choose a reason for hiding this comment

orbeckst May 11, 2021

Choose a reason for hiding this comment

orbeckst May 11, 2021

Choose a reason for hiding this comment

orbeckst May 11, 2021

Choose a reason for hiding this comment

alejob May 14, 2021 • edited Loading

Choose a reason for hiding this comment

orbeckst commented Apr 14, 2022

PicoCentauri commented Apr 14, 2022 • edited Loading

IAlibay commented May 16, 2022

PicoCentauri commented May 17, 2022

pep8speaks commented Apr 28, 2021 •

edited

Loading

codecov bot commented Apr 28, 2021 •

edited

Loading

alejob commented May 10, 2021 •

edited

Loading

alejob May 14, 2021 •

edited

Loading

PicoCentauri commented Apr 14, 2022 •

edited

Loading