Waterdynamics.SurvivalProbability start/stop/step/tau_max #2525

IAlibay · 2020-02-10T18:17:00Z

Fixes #2524

Changes made in this Pull Request:

Removes the t0, tf, and dtmax keywords from the :class:Waterdynamics.SurvivalProbability constructor.
Makes the stop keyword for :meth:WaterDynamics.SurvivalProbability.run exclusive rather than inclusive.

PR Checklist

Tests? (using existing tests, just ammending stop to be 1 higher).
Docs?
CHANGELOG updated?
Issue raised/referenced?

codecov · 2020-02-10T19:38:02Z

Codecov Report

Merging #2525 into develop will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #2525      +/-   ##
===========================================
+ Coverage    91.14%   91.19%   +0.04%     
===========================================
  Files          174      174              
  Lines        23717    23700      -17     
  Branches      3104     3099       -5     
===========================================
- Hits         21618    21614       -4     
+ Misses        1480     1472       -8     
+ Partials       619      614       -5

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/waterdynamics.py	`93.93% <100.00%> (+3.14%)`	⬆️

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 41ebfdf...63680fe. Read the comment docs.

richardjgowers · 2020-02-12T10:34:08Z

package/MDAnalysis/analysis/waterdynamics.py

        tau_max : int, optional
-            Survival probability is calculated for the range 1 <= `tau` <= `tau_max`
+            Survival probability is calculated for the range
+            1 <= `tau` <= `tau_max`.


If stop is exclusive does this docstring need updating?

Not sure here. tau_max should be independent of stop, i.e. previously the case was that stop was set in the code to be stop+1. The changes done here only change it so that stop=stop, tau and tau_max's behaviour shouldn't be affected (if that makes sense).

Exactly, tau_max is independent of stop.

richardjgowers

Does the exclusive/inclusive change line this class up with the others?

IAlibay · 2020-02-28T09:06:03Z

Does the exclusive/inclusive change line this class up with the others?

@richardjgowers I think I have pretty much failed to understand what you meant from your review 😅 Any chance you could add some more info?

richardjgowers · 2020-02-28T09:17:23Z

You made stop exclusive (or inclusive) when it was the other way around. Is it now like other classes?

…

On Fri, Feb 28, 2020 at 09:06, Irfan Alibay ***@***.***> wrote: Does the exclusive/inclusive change line this class up with the others? @richardjgowers <https://github.com/richardjgowers> I think I have pretty much failed to understand what you meant from your review 😅 Any chance you could add some more info? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2525?email_source=notifications&email_token=ACGSGBZKBAWHD5BEWM2RKKLRFDHXXA5CNFSM4KSTDACKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENHZS2Y#issuecomment-592419179>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACGSGB534BEJDUDTUVD7K7LRFDHXXANCNFSM4KSTDACA> .

IAlibay · 2020-03-01T16:19:31Z

You made stop exclusive (or inclusive) when it was the other way around. Is it now like other classes?

@richardjgowers Yeah, so as per #2505 (review) the idea in making it exclusive was to align it with the AnalysisBase's exclusive stop behaviour:

mdanalysis/package/MDAnalysis/analysis/base.py

Lines 179 to 180 in 977fb79

    
           for i, ts in enumerate( 
        
                   self._trajectory[self.start:self.stop:self.step]):

With regards to the other waterdynamics classes, unfortunately I'm not sure. I'm having a bit of a hard time working out how HydrogenBondLifetimes's tf keyword is working, but it looks like it's exclusive?

For WaterOrientationalRelaxation, this line seems to indicate that self.tf (passed to totalFrames) is exclusive (similar thing for MeanSquareDisplacement):

mdanalysis/package/MDAnalysis/analysis/waterdynamics.py

Line 784 in 977fb79

for j in range(totalFrames // dt - 1):

orbeckst · 2020-03-13T00:27:32Z

@alejob could you please also review this PR on WaterDynamics?

There were a few questions #2525 (comment) and maybe you as one of the authors can answer them.

Thanks!

alejob · 2020-03-16T05:43:13Z

Ok, let me check this and I'll answer in a couple of days. I didn't realize all the current changes.

IAlibay · 2020-03-29T16:18:02Z

Ok, let me check this and I'll answer in a couple of days. I didn't realize all the current changes.

Just re-pinging @alejob as a reminder for review. I do realise things are a little bit (that's probably an understatement) chaotic for everyone, so no worries if you don't have time to review this.

alejob · 2020-04-03T06:58:02Z

Hey there!
All the module was written with inclusion in mind, regarding the tf parameter.

With regards to the other waterdynamics classes, unfortunately I'm not sure. I'm having a bit of a hard time working out how HydrogenBondLifetimes's tf keyword is working, but it looks like it's exclusive?

It is also inclusive, in the code it is define as

mdanalysis/package/MDAnalysis/analysis/waterdynamics.py

Line 515 in 977fb79

self.tf = tf - 1

but later I use this in this way

mdanalysis/package/MDAnalysis/analysis/waterdynamics.py

Line 573 in 977fb79

if (t0 + dt <= tf):

tf is a value fill by self.tf.

For WaterOrientationalRelaxation, this line seems to indicate that self.tf (passed to totalFrames) is exclusive (similar thing for MeanSquareDisplacement):

The -1 is because the list start in zero, so if there is not -1, there will be a "list index out of bound".

mdanalysis/package/MDAnalysis/analysis/waterdynamics.py

Line 784 in 977fb79

for j in range(totalFrames // dt - 1):

I hava a question about this PR, this is only for make it consistent with the AnalysisBase? I'm pretty sure I lost a discussion about this, but does this PR improve something in the usability of this class? and also, If I want to obtain the same results that this module delivers, with the new implementation I should add an unit to the tf value? ex: If before this PR I wanted to calculate something until the frame 1000, now should I write 1001?

IAlibay · 2020-04-08T11:25:33Z

Thanks for having a look through @alejob

It seems like I've completely failed to understand the behaviour of tf for most of these classes, I'll have a longer look at this.

I hava a question about this PR, this is only for make it consistent with the AnalysisBase?

So there are two aims to this PR.

Probably the most important thing re: v1.0 here is to remove the deprecated t0, tf, and dtmax keywords from the :Waterdynamics.SurvivalProbability.
As pointed out by @orbeckst (Remove start/stop/step from AnalysisBase kwargs #2505 (review)) there is also a descrepancy between how Waterdynamics treats stop relative to how AnalysisBase does. So this PR aims to at least fix this for SurvivalProbability (although the question was raised as to whether or not this should be done for the whole of WaterDynamics).

I'm pretty sure I lost a discussion about this, but does this PR improve something in the usability of this class?

Whilst the current behaviour is documented in SurvivalProbability, one could see, especially considering my above failure to understand tf across most of the classes, how a user error could come up here. So the view here is that making things similar to AnalysisBase would avoid these kinds of errors / allow for a more consistent API.

and also, If I want to obtain the same results that this module delivers, with the new implementation I should add an unit to the tf value? ex: If before this PR I wanted to calculate something until the frame 1000, now should I write 1001?

Indeed, assuming one had frames ranging from 0 to 1000, then if passed explicitly stop should be equal to 1001 (or higher) to cover the full range of frames. As far as I am aware, this is the current way in which AnalysisBase works.

orbeckst · 2020-05-12T14:34:10Z

@IAlibay, I just merged PR #2256 which changed waterdynamics. Can you do resolve the conflicts and have a look again?

orbeckst · 2020-05-15T17:41:03Z

This looks mostly done (bringing WaterAnalysis in-line with the other analysis classes) although the following seems needed:

resolving conflicts (might not be trivial so @IAlibay should do it)
@richardjgowers 's blocking review – I think @IAlibay answered his question (now all exclusive, in line with AnalysisBase use) but please check that you're satisfied
@alejob can you please do a formal review of the changes with an "approve" or "changes requested"?
I'll be happy to do a review after @alejob – but I don't know the code as well as the original author so I'll mostly follow @alejob 's lead.

I added it to the 1.0 milestone so let's get this done.

IAlibay · 2020-05-15T17:47:32Z

Sorry about the delay here @orbeckst, I've got a slot in my calendar specifically to look at this tomorrow :)

A thought, do we want to extend this beyond survival probability to make stop exclusive across all of waterdynamics?

orbeckst · 2020-05-15T18:48:43Z

A thought, do we want to extend this beyond survival probability to make stop exclusive across all of waterdynamics?

That would be great – even if we don't have everything AnalysisBase-based yet, having a consistent interface is necessary. And with semantic versioning, backwards-incompatible changes in the API are very annoying because you need to bump the major version. Changing the meaning of the stop argument is such a major API change because it will lead to wrong results between different versions. I don't think anyone wants to go to MDA 2.0 just in order to update the arguments in waterdynamics...

orbeckst · 2020-05-15T18:51:08Z

Sorry about the delay here

Oh, forgot: please do not apologize!!!!! You're doing an awful lot of work here, thank you, thank you!

I see it as my job to occasionally check on PRs and help shepherd them to completion so that all the work that has already gone into them also counts. Sometimes one just has to clarify what needs doing.

orbeckst · 2020-05-15T18:52:41Z

A thought, do we want to extend this beyond survival probability to make stop exclusive across all of waterdynamics?

I would open a new PR, though, just to get this one done. It can then serve as a blueprint for the rest.

As we're preaching to our GSoC students: many small PRs are better than one big one ;-).

IAlibay · 2020-05-27T12:46:07Z

The conflicts should now be solved. Pinging @richardjgowers and @alejob for review.

orbeckst · 2020-05-29T08:06:25Z

@alejob your review is important here. Do you have an idea when you might be able to do it? Thanks!

alejob · 2020-05-29T15:48:23Z

Sorry for the late! Today at night I'll Look this. El vie., 29 may. 2020 04:06, Oliver Beckstein <[email protected]> escribió:

…

@alejob <https://github.com/alejob> your review is important here. Do you have an idea when you might be able to do it? Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2525 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACBMOONQ5IYBHXFEFLB7PTDRT5UI7ANCNFSM4KSTDACA> .

alejob

I understand that is necessary to have a module that is compatible with the AnalysisBase, so I'm very happy with the general idea.

In the other side I don't see changes on the examples of documentation (or a comment), please change that, because it will be the only updated class and that could be confusing.

alejob · 2020-05-30T05:31:13Z

package/MDAnalysis/analysis/waterdynamics.py

-
-
-    def run(self, tau_max=20, start=0, stop=None, step=1, residues=False, intermittency=0, verbose=False):
+    def run(self, tau_max=20, start=None, stop=None, step=None, residues=False,


There is no longer backward compatibility warnings? why?

@alejob by this do you mean the removal of t0, tf, and dtmax? These have been slated for deprecation for a long time. As part of the upcoming 1.0.0 release, we are aiming to remove all historical deprecated code.

alejob · 2020-05-30T05:45:07Z

package/MDAnalysis/analysis/waterdynamics.py

        tau_max : int, optional
-            Survival probability is calculated for the range 1 <= `tau` <= `tau_max`
+            Survival probability is calculated for the range
+            1 <= `tau` <= `tau_max`.


Exactly, tau_max is independent of stop.

Adds more documentation to detail that :class:`SurvivalProbability` now has an exclusive behaviour unike the other :mod:`MDAnalysis.analysis.waterdynamics` classes.

IAlibay · 2020-05-30T07:46:36Z

In the other side I don't see changes on the examples of documentation (or a comment), please change that, because it will be the only updated class and that could be confusing.

Thanks for the review @alejob. I have included documentation (beyond what was already in :meth:SurvivalProbability.run. Let me know if this is what you had in mind & if you think it needs changes.

alejob

Great, thank you!

orbeckst · 2020-05-31T23:28:15Z

Ping @richardjgowers for final approval?

IAlibay added 4 commits February 10, 2020 17:23

removes deprecated keywords

d3a052b

Aligns traj control to AnalysisBase methods

5efc8cb

keyword removal

dd16e4b

Update CHANGELOG

9c9e453

richardjgowers reviewed Feb 12, 2020

View reviewed changes

richardjgowers requested changes Feb 12, 2020

View reviewed changes

orbeckst requested a review from alejob March 13, 2020 00:26

Merge branch 'develop' into waterdynamics-sp-startstop

8281522

orbeckst added this to the 1.0 milestone May 15, 2020

lilyminium mentioned this pull request May 20, 2020

Parameters not saved in Analysis classes #2206

Closed

Merge branch 'develop' into waterdynamics-sp-startstop

6cb1f6c

Merge branch 'develop' into waterdynamics-sp-startstop

3bd4fe9

alejob suggested changes May 30, 2020

View reviewed changes

IAlibay added 2 commits May 30, 2020 08:17

Merge branch 'develop' into waterdynamics-sp-startstop

31f1ea7

Adds documentation of exclusive behaviour

63680fe

Adds more documentation to detail that :class:`SurvivalProbability` now has an exclusive behaviour unike the other :mod:`MDAnalysis.analysis.waterdynamics` classes.

alejob approved these changes May 30, 2020

View reviewed changes

richardjgowers approved these changes Jun 1, 2020

View reviewed changes

richardjgowers merged commit 0911ff3 into MDAnalysis:develop Jun 1, 2020

IAlibay mentioned this pull request Nov 3, 2023

Make waterdynamics stop behaviour the same as AnalysisBase MDAnalysis/waterdynamics#22

Open

fiona-naughton added maintainability Component-Analysis deprecation Deprecated functionality to give advance warning for API changes. labels Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Waterdynamics.SurvivalProbability start/stop/step/tau_max #2525

Waterdynamics.SurvivalProbability start/stop/step/tau_max #2525

IAlibay commented Feb 10, 2020

codecov bot commented Feb 10, 2020 •

edited

Loading

richardjgowers Feb 12, 2020

IAlibay Feb 12, 2020

alejob May 30, 2020

richardjgowers left a comment

IAlibay commented Feb 28, 2020

richardjgowers commented Feb 28, 2020 via email

IAlibay commented Mar 1, 2020 •

edited

Loading

orbeckst commented Mar 13, 2020

alejob commented Mar 16, 2020

IAlibay commented Mar 29, 2020

alejob commented Apr 3, 2020

IAlibay commented Apr 8, 2020

orbeckst commented May 12, 2020

orbeckst commented May 15, 2020

IAlibay commented May 15, 2020

orbeckst commented May 15, 2020

orbeckst commented May 15, 2020

orbeckst commented May 15, 2020

IAlibay commented May 27, 2020

orbeckst commented May 29, 2020

alejob commented May 29, 2020 via email

alejob left a comment

alejob May 30, 2020

IAlibay May 30, 2020

alejob May 30, 2020

IAlibay commented May 30, 2020

alejob left a comment •

edited

Loading

orbeckst commented May 31, 2020



		def run(self, tau_max=20, start=0, stop=None, step=1, residues=False, intermittency=0, verbose=False):
		def run(self, tau_max=20, start=None, stop=None, step=None, residues=False,

Waterdynamics.SurvivalProbability start/stop/step/tau_max #2525

Waterdynamics.SurvivalProbability start/stop/step/tau_max #2525

Conversation

IAlibay commented Feb 10, 2020

PR Checklist

codecov bot commented Feb 10, 2020 • edited Loading

Codecov Report

richardjgowers Feb 12, 2020

Choose a reason for hiding this comment

IAlibay Feb 12, 2020

Choose a reason for hiding this comment

alejob May 30, 2020

Choose a reason for hiding this comment

richardjgowers left a comment

Choose a reason for hiding this comment

IAlibay commented Feb 28, 2020

richardjgowers commented Feb 28, 2020 via email

IAlibay commented Mar 1, 2020 • edited Loading

orbeckst commented Mar 13, 2020

alejob commented Mar 16, 2020

IAlibay commented Mar 29, 2020

alejob commented Apr 3, 2020

IAlibay commented Apr 8, 2020

orbeckst commented May 12, 2020

orbeckst commented May 15, 2020

IAlibay commented May 15, 2020

orbeckst commented May 15, 2020

orbeckst commented May 15, 2020

orbeckst commented May 15, 2020

IAlibay commented May 27, 2020

orbeckst commented May 29, 2020

alejob commented May 29, 2020 via email

alejob left a comment

Choose a reason for hiding this comment

alejob May 30, 2020

Choose a reason for hiding this comment

IAlibay May 30, 2020

Choose a reason for hiding this comment

alejob May 30, 2020

Choose a reason for hiding this comment

IAlibay commented May 30, 2020

alejob left a comment • edited Loading

Choose a reason for hiding this comment

orbeckst commented May 31, 2020

codecov bot commented Feb 10, 2020 •

edited

Loading

IAlibay commented Mar 1, 2020 •

edited

Loading

alejob left a comment •

edited

Loading