Fixed a bug that appear when there are no water molecules in selection. #1322

alejob · 2017-04-26T19:28:24Z

Changes made in this Pull Request:

CHANGELOG updated.
Improved print to screen format.
Fixed orthography.

PR Checklist

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

Improved print to screen format. Fixed some orthography. CHANGELOG updated.

alejob · 2017-04-26T20:25:08Z

I'm having this error in Travis CI:

...
Ran 4504 tests in 430.775s

OK (KNOWNFAIL=4, SKIP=11)

No output has been received in the last 10m0s, this potentially indicates a stalled
build or something wrong with the build itself.

Check the details on how to adjust your build configuration on:
https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

The build has been terminated

The build stop in one "Build Jobs" , but in my PC it builds OK.

Any idea?

orbeckst · 2017-04-27T03:41:36Z

We have been experiencing these stalled jobs recently and are not sure why. Restarting the job typically does it.

(Given that your travis check passed, you or someone else probably restarted it, or you submitted another commit that triggered the tests again.)

orbeckst

Please correct comments.
Must add a test case for the specific problem that you are fixing.

orbeckst · 2017-04-27T03:44:58Z

package/MDAnalysis/analysis/waterdynamics.py

@@ -820,7 +825,8 @@ def _selection_serial(self, universe, selection_str):
        selection = []
        for ts in universe.trajectory:
            selection.append(universe.select_atoms(selection_str))
-            print(ts.frame)
+            sys.stdout.write("\rSelecting atoms from frame: %i"%ts.frame)


Do not use sys.stdout.write: if you want to show progress, use our own lib.log.ProgressMeter class.

Or a good old print.

ProgressMeter is made for this use case. Bare prints are not good because you cannot switch them off easily (short of redirecting stdout).

There should be a verbosity argument to the function. I honestly never look at the log and I'm not sure how many people know about it.

Maybe we misunderstand each other (?): I am not advocating to use a logger but use the ProgressMeter class which properly encapsulates the use case here: write status for every iteration in a loop. Under the hood it uses sys.stderr and flushes but with added goodies like #928 / #960 (and some other neat things that @jbarnoud has shown or promised (IIRC... ;-) ) such as real progress bars, including nice ones in Jupyter notebooks) it makes sense to unify the way we show progress.

And yes, add the standard verbose=True argument and pass it to ProgressMeter.

orbeckst · 2017-04-27T03:46:02Z

package/MDAnalysis/analysis/waterdynamics.py

@@ -415,6 +415,7 @@
 from six.moves import range, zip_longest

 import numpy as np
+import sys


You should not need sys for anything. For output, use lib.log.ProgressMeter (see below comment).

orbeckst · 2017-04-27T03:48:04Z

package/MDAnalysis/analysis/waterdynamics.py

-        return (valOH,valHH,valdip)
+
+        if n == 0:
+            return (0,0,0)


Use PEP 8 formatting (space after comma) – applies to the whole file.

Note: If you start formatting the whole file according to PEP 8 do this in a separate commit so that it is clear that this commit only contains formatting. However, any new code that you're adding/changing should already be formatted to PEP 8.

Ok, I'll adapt the code to the PEP8 format, but is it necessary to have lines shorter than 80 column even if I lost readability of code?

Try to keep a limit at 80-ish columns. There is no point at cutting lines at 79 for the sake of it, 82 or 85 as just as readable, but long lines are often the sign of other issues.

We decided to go with 79 characters, see Style Guide: Coding Style – there was quite a bit of discussion preceding this decision. Generally speaking, I find that most code still looks fine when broken sensibly across lines.

A lot of old code still does not conform to PEP 8 but we are slowly working to making this more uniform so at least newly added/edited code should conform.

In the end, a unified appearance makes it easier for other people to understand the code and it can make merging easier, too.

orbeckst · 2017-04-27T03:50:14Z

package/MDAnalysis/analysis/waterdynamics.py

+            valOH = valOH/n
+            valHH = valHH/n
+            valdip = valdip/n
+            return (valOH,valHH,valdip)


How about leaving the return at the end?

valOH, valHH, valdip = (valOH/n, valHHn, valdip/n) if n > 0 else (0, 0, 0) return valOH, valHH, valdip

or even

return (valOH/n, valHH/n, valdip/n) if n > 0 else (0, 0, 0)

Ok, I like it.

orbeckst · 2017-04-27T03:52:10Z

package/MDAnalysis/analysis/waterdynamics.py

+        if n == 0:
+            return (0,0,0)
+        else:
+            return (sumDeltaOH/n,sumDeltaHH/n,sumDeltadip/n)


I like return at the end better than hidden in an if/else. But ultimately your choice.

orbeckst · 2017-04-27T03:54:48Z

package/MDAnalysis/analysis/waterdynamics.py

+        if n == 0:
+            return (0,0,0)
+        else:
+            return (sumDeltaO/n)


no parentheses, return at end?

orbeckst · 2017-04-27T03:54:58Z

package/MDAnalysis/analysis/waterdynamics.py

@@ -1102,7 +1119,8 @@ def _selection_serial(self,universe,selection_str):
        selection = []
        for ts in universe.trajectory:
            selection.append(universe.select_atoms(selection_str))
-            print(ts.frame)
+            sys.stdout.write("\rSelecting atoms from frame: %i"%ts.frame)


use ProgressMeter

orbeckst · 2017-04-27T03:55:52Z

package/MDAnalysis/analysis/waterdynamics.py

+
+        #if no water molecules remain in selection, there is nothing to get the mean, so n = 0.
+        if n == 0:
+            return (0,0,0)


This is weird: isn't sumDeltaP a scalar? Then why would you return a tuple for n==0??

orbeckst · 2017-04-27T03:56:40Z

package/MDAnalysis/analysis/waterdynamics.py

+
+        #if no water molecules remain in selection, there is nothing to get the mean, so n = 0.
+        if n == 0:
+            return (0,0,0)


Why a tuple for n==0?? Isn't sumDelta0 a scalar?

You're right, my mistake, next time I'll implement tests for this "little" changes.

orbeckst · 2017-04-27T03:57:01Z

package/MDAnalysis/analysis/waterdynamics.py

@@ -1218,7 +1240,8 @@ def _selection_serial(self,universe,selection_str):
        selection = []
        for ts in universe.trajectory:
            selection.append(universe.select_atoms(selection_str))
-            print(ts.frame)
+            sys.stdout.write("\rSelecting atoms from frame: %i"%ts.frame)
+            sys.stdout.flush()


ProgressMeter instead

alejob · 2017-04-27T14:17:05Z

Thanks for all your comments, I'll work on this.

Changed print to screen, now using ProgressMeter. Avoiding hidden return inside if/else. Added test to fixed bug.

orbeckst · 2017-04-28T05:55:54Z

@alejob all looking good, nice work! However, I wanted to consolidate your commits so I created PR #1323 . I am closing this PR and we continue at the other one.

Fixed a bug that appear when there are no water molecules in selection.

8d6c348

Improved print to screen format. Fixed some orthography. CHANGELOG updated.

orbeckst requested changes Apr 27, 2017

View reviewed changes

alejob added 3 commits April 27, 2017 12:26

Formatted according to PEP8

8862ec2

CHANGELOG updated.

12f0905

Changed print to screen, now using ProgressMeter. Avoiding hidden return inside if/else. Added test to fixed bug.

Adapted to PEP8, not fully, but most

b92d638

orbeckst self-assigned this Apr 28, 2017

orbeckst mentioned this pull request Apr 28, 2017

wateranalysis: fix for empty selections and PEP8 #1323

Merged

3 tasks

orbeckst approved these changes Apr 28, 2017

View reviewed changes

orbeckst closed this Apr 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed a bug that appear when there are no water molecules in selection. #1322

Fixed a bug that appear when there are no water molecules in selection. #1322

alejob commented Apr 26, 2017 •

edited

Loading

alejob commented Apr 26, 2017

orbeckst commented Apr 27, 2017

orbeckst left a comment

orbeckst Apr 27, 2017

kain88-de Apr 27, 2017

orbeckst Apr 27, 2017

kain88-de Apr 27, 2017

orbeckst Apr 27, 2017

orbeckst Apr 27, 2017

orbeckst Apr 27, 2017

orbeckst Apr 27, 2017

alejob Apr 27, 2017

jbarnoud Apr 27, 2017 •

edited

Loading

orbeckst Apr 27, 2017

orbeckst Apr 27, 2017 •

edited

Loading

alejob Apr 27, 2017

orbeckst Apr 27, 2017

orbeckst Apr 27, 2017

orbeckst Apr 27, 2017

orbeckst Apr 27, 2017

orbeckst Apr 27, 2017

alejob Apr 27, 2017

orbeckst Apr 27, 2017

alejob commented Apr 27, 2017

orbeckst commented Apr 28, 2017

Fixed a bug that appear when there are no water molecules in selection. #1322

Fixed a bug that appear when there are no water molecules in selection. #1322

Conversation

alejob commented Apr 26, 2017 • edited Loading

PR Checklist

alejob commented Apr 26, 2017

orbeckst commented Apr 27, 2017

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbarnoud Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alejob commented Apr 27, 2017

orbeckst commented Apr 28, 2017

alejob commented Apr 26, 2017 •

edited

Loading

jbarnoud Apr 27, 2017 •

edited

Loading

orbeckst Apr 27, 2017 •

edited

Loading