Move the water bridge analysis to the new analysis class by xiki-tempula · Pull Request #2087 · MDAnalysis/mdanalysis

xiki-tempula · 2018-10-03T09:45:51Z

Move the water bridge analysis to the new analysis class, add high order water bridge support.
Rewritte the count_by_type and count_by_time function, add custom anaylsis function support.

PR Checklist

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

Move the water bridge analysis to the new analysis class, add high Order water bridge support. (PR #2087)

xiki-tempula · 2018-10-03T11:00:34Z

It seems that the line result = [(*key, result_dict[key]*1.0/length) for key in result_dict] is falling in the older versions of python. I wonder if there are any hacks to go around this problem except writing another loop?

kain88-de · 2018-10-03T12:24:12Z

yes this is not valid syntax for python 3.4 and older. What did you want to replace?

xiki-tempula · 2018-10-03T13:27:14Z

@kain88-de
My current thought would be replaced with

result = [[i for i in key] for key in result_dict]
[result[i].append(result_dict[key]*1.0/length) for i, key in enumerate(result_dict)]
return result

kain88-de · 2018-10-03T14:00:05Z

what are the keys of `result_dict`?

…

On Wed, Oct 3, 2018 at 3:27 PM xiki-tempula ***@***.***> wrote: @kain88-de <https://github.com/kain88-de> My current thought would be replaced with result = [[i for i in key] for key in result_dict] [result[i].append(result_dict[key]*1.0/length) for i, key in enumerate(result_dict)] return result — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2087 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEGnVvmHJBsNHoOnvh6sk31QJCrgmVvdks5uhLszgaJpZM4XFqGz> .

xiki-tempula · 2018-10-03T14:02:42Z

@kain88-de It is a tuple has the form
(from_index, to_index, (from_resname, from_resid, from_name), (to_resname, to_resid, to_name)).

kain88-de · 2018-10-03T14:18:17Z

Why not a manual unpack if the structure is known? `[[k[0], k[1], k[2], k[3], v/length] for k, v in six.iteritems(result_dict)]` To remove `*1.0` include `from __future__ import division` at the beginning of the file. Python will then automatically do a type conversion to float.

…

On Wed, Oct 3, 2018 at 4:02 PM xiki-tempula ***@***.***> wrote: @kain88-de <https://github.com/kain88-de> It is a tuple has the form (from_index, to_index, (from_resname, from_resid, from_name), (to_resname, to_resid, to_name)). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2087 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEGnViOslnNCbQEONH0y82eUZLDea0fVks5uhMODgaJpZM4XFqGz> .

xiki-tempula · 2018-10-03T14:22:00Z

@kain88-de Sorry, I haven't made this clear. This is the default output version. In this update, the user has the freedom to define the key to this dictionary.
The current output is formatted so that it is compatible with the previous version. Since the output is not used by other module in the mda. Perhaps, I could just do [[k, v/length] for k, v in six.iteritems(result_dict)].

package/MDAnalysis/analysis/hbonds/wbridge_analysis.py

codecov · 2018-10-05T09:20:35Z

Codecov Report

Merging #2087 into develop will increase coverage by 0.07%.
The diff coverage is 91.05%.

@@             Coverage Diff             @@
##           develop    #2087      +/-   ##
===========================================
+ Coverage    89.75%   89.82%   +0.07%     
===========================================
  Files          173      173              
  Lines        21536    21711     +175     
  Branches      2804     2846      +42     
===========================================
+ Hits         19329    19502     +173     
  Misses        1615     1615              
- Partials       592      594       +2

Impacted Files	Coverage Δ
...age/MDAnalysis/analysis/hbonds/wbridge_analysis.py	`91.54% <91.05%> (+5.09%)`	⬆️

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 7854011...ebf2fb2. Read the comment docs.

orbeckst · 2018-11-15T01:22:27Z

The doc test had stalled. I restarted it. Then we should also see coverage.

Please also fix the conflicts with develop.

xiki-tempula · 2018-11-21T12:16:48Z

@orbeckst Thank you for the advice. I have fixed the conflict but I think the py36 develop build might have some problem which makes the test fails.

orbeckst · 2018-11-21T18:58:14Z

I restarted the 3.6 jobs, maybe that helps?

orbeckst · 2018-11-21T18:58:37Z

If not, drop an email to the dev list; I am not sure what's wrong.

orbeckst · 2018-11-21T22:58:39Z

I just merged @richardjgowers PR #2145 so rebase against develop (or merge develop) and see if this fixes things.

xiki-tempula · 2018-11-21T23:47:25Z

@orbeckst Thank you for the information. Would you mind restart the Travis build, please? I guess there we probably don't need another commit.

orbeckst · 2018-11-22T01:36:42Z

Did you merge develop? This is needed for the change to take place. This will automatically run Travis.

…to new_wba

xiki-tempula · 2018-11-22T17:11:22Z

@orbeckst Thank you for the advice. I have merged the develop to this PR and it seems that MDAnalysisTests.analysis.test_encore.TestEncore is failing.

orbeckst · 2018-11-22T17:55:39Z

Probably the usual failure due to the sensitivity of the encore tests to small data. Perhaps someone who's not celebrating Thanksgiving today (big deal in the US) could restart the Travis tests?

…

-- Oliver Beckstein email: orbeckst@gmail.com

Am Nov 22, 2018 um 10:11 schrieb xiki-tempula ***@***.***>: @orbeckst Thank you for the advice. I have merged the develop to this PR and it seems that MDAnalysisTests.analysis.test_encore.TestEncore is failing. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

orbeckst · 2018-11-27T00:12:58Z

I restarted the one job that failed.

orbeckst

That's a big PR. It mixes new features (higher order wbridges) with refactoring (AnalysisBase). It would have been easier if these two things had been addressed separately. In particular, new features automatically imply that this has to go into a 0.20.x (because we use semantic versioning.

Many of my comments are regarding documentation, mainly to make clearer what happens in the module.

I am also concerned with code duplication between the new wbridge analysis and the hbond analysis. I am not 100% sure what the best course forward is but I'd like to hear suggestions. Perhaps create a mixin-class and raise an issue for refactoring hbond analysis; not everything has to be accomplished in one PR (many smaller PRs with weel-defined goals are much better).

In the tests I'd also suggest to reduce code-duplication by using more fixtures as this also leads to cleaner separation of data and test logic.

ADDENDUM

remove file package/.DS_Store from the PR
rebase against develop (or merge develop)

testsuite/MDAnalysisTests/analysis/test_wbridge.py

orbeckst · 2018-12-06T20:50:57Z

testsuite/MDAnalysisTests/analysis/test_wbridge.py

+            raise pytest.fail("selection_type aaa should rasie error")
+
+    def test_empty_selection(self):
+        grofile = '''Test gro file


make it a fixture

orbeckst · 2018-12-06T20:51:04Z

testsuite/MDAnalysisTests/analysis/test_wbridge.py

+
+    def test_loop(self):
+        '''Test if loop can be handled correctly'''
+        grofile = '''Test gro file


make it a fixture

testsuite/MDAnalysisTests/analysis/test_wbridge.py

orbeckst · 2018-12-06T21:44:58Z

testsuite/MDAnalysisTests/analysis/test_wbridge.py

+    return Atomtypes(np.array([guess_atom_type(name) for name in names], dtype=object))
+
+
+class TestHydrogenBondAnalysis(object):


Shouldn't this be in a different test?

Or is the name wrong?

@orbeckst I'm thinking of unifying the output of HydrogenBondAnalysis and WaterBridgeAnalysis. So I have transported the HydrogenBondAnalysis to here to make sure WaterBridgeAnalysis can pass the tests which were written for HydrogenBondAnalysis.

testsuite/MDAnalysisTests/analysis/test_wbridge.py

…into new_wba

xiki-tempula · 2019-04-07T09:26:04Z

@orbeckst Sorry for bothering you in this busy season of GSoC, I have submitted a commit which passes all the test. Would you mind give a review, please? Thank you. Once this PR is merged, we can probably be thinking of how to use capped_distance to improve the performance.

orbeckst · 2019-07-31T17:14:18Z

@xiki-tempula , can you please go through the comments above and for each comment reply how you addressed it or if you decided to not address it an why? Think of it as addressing revisions in a paper. Don't click the "Resolved" button on the comments. I will do this after having read your replies.

Given the decisions regarding refactoring H-bond analysis in #2238 , you don't need to worry about code duplication between HydrogenBondAnalysis and WaterBridgeAnalysis for the moment. This might come up later with the new module.

Reading your summary of changes will make it a lot easier for me to go through things. The PR is so big that I don't have the time to re-read all the lines of code and compare to what I said in December. (As a general rule: the smaller the PR the better the chance that someone can review it in a timely manner.)

Ping me when you're done and I will review your comments and code where necessary.

Thanks!

xiki-tempula · 2019-08-05T20:46:14Z

@orbeckst Thank you for the comments. I have added my comments to the comments. Most of the issue is with the use of the fixture. Now, all the universe used in the test section is made into a fixture and is listed on the top of the test section.

There are some lines in the test which exceeds 80 characters. I can fix that but I prefer to do that when things are finalised.

orbeckst

Looking good, just address these minor issues (see comments):

remove .DS_Store files – add .DS_Store to .gitignore
small documentation issues (see comments)
very minor code style issue

orbeckst · 2019-08-09T23:20:21Z

package/MDAnalysis/analysis/hbonds/wbridge_analysis.py

           # hbonds linking the selection 1 and selection 2 to the bridging
           # water 1
           [ # hbond 1 from selection 1 to the bridging water 1
-              <donor index (0-based)>,


add the versionchanged to the class

add an entry to CHANGELOG's Changes stating that the output format of WaterBridgeAnalysis changed

orbeckst · 2019-08-09T23:23:04Z

package/MDAnalysis/analysis/hbonds/wbridge_analysis.py


-.. _pandas.DataFrame: http://pandas.pydata.org/pandas-docs/stable/generated/pandas.DataFrame.html
+Note that the result is arranged in the format of (key, proportion of time). When no
+custom analysis function is supplied, the key is expended for backward compatibility.


fix expended -> expanded (?)

state what it is backward compatible to

orbeckst · 2019-08-09T23:24:44Z

package/MDAnalysis/analysis/hbonds/wbridge_analysis.py

+      key = (s1_resname, s1_resid, s2_resname, s2_resid)
+      output[key] += 1
+
+  w.count_by_type(analysis_func=analysis)


Mention the "in-place" modification requirement for the analysis() function.

package/MDAnalysis/analysis/hbonds/wbridge_analysis.py

orbeckst · 2019-08-09T23:31:37Z

package/MDAnalysis/analysis/hbonds/wbridge_analysis.py

-            As the _timeseries to timeserie conversion will be deprecated in 1.0
-            this function will automatically lose its value.
-        """
+    def _expand_timeseries(self, entry, output_format=None):


What does "old" mean? Make this precise with version information, e.g. "format for release up to 0.19.2"

The explanation has been added.

orbeckst · 2019-08-09T23:32:09Z

package/MDAnalysis/analysis/hbonds/wbridge_analysis.py

+                # atom1 is hydrogen bond donor, position not swapped.
+                atom1, atom2 = atom1, atom2
+        else:
+            raise KeyError('Only \'sele1_sele2\' or \'donor_acceptor\' are allowed as output format.')


Use double quotation marks to get rid of ugly backslash escapes.

Double quotations added.

orbeckst · 2019-08-09T23:33:27Z

package/MDAnalysis/analysis/hbonds/wbridge_analysis.py

-        See Also
-        --------
-        :attr:`table` : structured array of the data
+        .. versionchanged:: 0.20.0


say what changed

.. versionchanged:: 0.20.0 The output format was changed bla bla...

The change has been mentioned

orbeckst · 2019-08-09T23:38:47Z

I merged develop into it and fixed the merge conflict in CHANGELOG; note that your GitHub handle @xiki-tempula was already in the authors list for 0.20.0.

xiki-tempula · 2019-08-11T09:42:31Z

@orbeckst I have made the corrections. All the lines in the test file which are too long are being altered. In the main file, though some of the lines are a bit too long, I think having them in the same line might make it easier to read.

orbeckst

That was a lot of work but finally: LGTM!

Thanks for being patient and addressing all issues.

Move the water bridge analysis to the new analysis class

4a453af

Move the water bridge analysis to the new analysis class, add high Order water bridge support. (PR #2087)

orbeckst reviewed Oct 4, 2018

View reviewed changes

package/MDAnalysis/analysis/hbonds/wbridge_analysis.py Show resolved Hide resolved

orbeckst reviewed Oct 4, 2018

View reviewed changes

package/MDAnalysis/analysis/hbonds/wbridge_analysis.py Show resolved Hide resolved

Fix pbc error

87b5c4e

xiki-tempula added 5 commits October 5, 2018 12:31

Add hydrogen bond testing to the water bridge test cases

0f56666

fixed test

3337e25

Change the wrong test file

0cae5fe

solve unordered table problem

ae9e386

remove test

635be54

xiki-tempula added 2 commits November 15, 2018 12:39

resolve conflict

9b28e58

Correct an error

f9921e5

Merge branch 'develop' of https://github.com/MDAnalysis/mdanalysis in…

0bf0dc1

…to new_wba

Crank up the test converage

f3f0013

orbeckst requested changes Dec 6, 2018

View reviewed changes

xiki-tempula added 9 commits December 18, 2018 11:37

Updated according to comments

2cc94e5

Merge branch 'develop' into new_wba

777e4e7

Fix the timeseries

1051c0c

Merge branch 'new_wba' of https://github.com/xiki-tempula/mdanalysis …

992b808

…into new_wba

remove tests from hydrogen bond analysis

ab7ed0c

a bit of error

160d564

Add a brief theory session

34b04e9

Update the doc to make it more clear

61a3762

minor update

b5791c5

xiki-tempula mentioned this pull request Feb 5, 2019

Custom analysis function for water bridge analysis #2198

Closed

p-j-smith mentioned this pull request Apr 5, 2019

Hbond analysis #2237

Merged

4 tasks

orbeckst mentioned this pull request Apr 6, 2019

refactoring HydrogenBondAnalysis #2238

Closed

xiki-tempula and others added 4 commits April 7, 2019 01:49

Update the doc and the underlying data

f20a904

Merge branch 'develop' into new_wba

26d09ea

fix bug

be3fde3

Merge branch 'new_wba' of https://github.com/xiki-tempula/mdanalysis …

fb5b840

…into new_wba

IAlibay mentioned this pull request Aug 2, 2019

Fixes PM issues (jlab and non-AnalysisBase) #2310

Merged

4 tasks

orbeckst requested changes Aug 9, 2019

View reviewed changes

Merge branch 'develop' into new_wba

8e8837f

Corrections being made

ebf2fb2

orbeckst approved these changes Aug 13, 2019

View reviewed changes

orbeckst merged commit e910a12 into MDAnalysis:develop Aug 13, 2019

xiki-tempula deleted the new_wba branch August 13, 2019 09:07

		return Atomtypes(np.array([guess_atom_type(name) for name in names], dtype=object))


		class TestHydrogenBondAnalysis(object):

Conversation

xiki-tempula commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

xiki-tempula commented Oct 3, 2018

Uh oh!

kain88-de commented Oct 3, 2018

Uh oh!

xiki-tempula commented Oct 3, 2018

Uh oh!

kain88-de commented Oct 3, 2018 via email

Uh oh!

xiki-tempula commented Oct 3, 2018

Uh oh!

kain88-de commented Oct 3, 2018 via email

Uh oh!

xiki-tempula commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

orbeckst commented Nov 15, 2018

Uh oh!

xiki-tempula commented Nov 21, 2018

Uh oh!

orbeckst commented Nov 21, 2018

Uh oh!

orbeckst commented Nov 21, 2018

Uh oh!

orbeckst commented Nov 21, 2018

Uh oh!

xiki-tempula commented Nov 21, 2018

Uh oh!

orbeckst commented Nov 22, 2018

Uh oh!

xiki-tempula commented Nov 22, 2018

Uh oh!

orbeckst commented Nov 22, 2018 via email

Uh oh!

orbeckst commented Nov 27, 2018

Uh oh!

orbeckst left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

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!

xiki-tempula commented Apr 7, 2019

Uh oh!

orbeckst commented Jul 31, 2019

Uh oh!

xiki-tempula commented Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

xiki-tempula commented Oct 3, 2018 •

edited

Loading

xiki-tempula commented Oct 3, 2018 •

edited

Loading

codecov bot commented Oct 5, 2018 •

edited

Loading

orbeckst left a comment •

edited

Loading

xiki-tempula commented Aug 5, 2019 •

edited

Loading