bug fix double counting water in Water bridge analysis

[xiki-tempula]

Fixes #3119

Changes made in this Pull Request:

Previously, Water bridge analysis counts the duplicated water via checking if the same hydrogen bond is being used.
For example,

        Acceptor(A1)···H(H2)−O(O3)···H(H7)-Donor(D8) 
                |            |
               H(H6)-O(O5)···H(H4)

Will give
A1···H2-O3
O3-H4···O5
O5-H6-A1
A1···H2-O3
O3···H7-D8.
This will get rejected as A1···H2-O3 appear twice. However, it doesn't check if O3-H4···O5 and O5···H4-O3 can exit at the same time and this PR fixed it.

PR Checklist

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

[pep8speaks]

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-05-07 06:59:13 UTC

[codecov]

Codecov Report

Merging #3120 (aaaec28) into develop (da46e84) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #3120   +/-   ##
========================================
  Coverage    93.03%   93.03%           
========================================
  Files          172      172           
  Lines        22724    22725    +1     
  Branches      3193     3194    +1     
========================================
+ Hits         21141    21142    +1     
  Misses        1533     1533           
  Partials        50       50           

Impacted Files	Coverage Δ
...nalysis/analysis/hydrogenbonds/wbridge_analysis.py	97.42% <100.00%> (+<0.01%)	⬆️

---

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 da46e84...aaaec28. Read the comment docs.

[tylerjereddy]

Mechanically, a new unit test has been added, the tests are passing, and the patch diff is 100 %.

I'm perhaps a little confused here because I'm not familiar with this analysis module, so I've asked a question about the desired order here.

[tylerjereddy]

The matching issue has "Will be recognised as a third-order water bridge" under the "expected behavior" section?

Is this supposed to be a "first order" water bridge because only one water actually contacts both the acceptor and donor? Or is it second order because of propagation into the "extra water" on the side?

[xiki-tempula]

@tylerjereddy I'm sorry. I didn't realise that it is in the expected behaviour section. I have amended the issue to reflect that.

[fiona-naughton]

lgtm - though (possible naive question), is there a reason for not doing the check for duplicates until the end rather than as you're building the routes? I would have thought the latter would be faster, but I may be wrong.

[xiki-tempula]

@fiona-naughton Thanks for the review.
This could surely be put at the end and my feeling is that given that we need to store such a network for each frame, removing the duplication before storing the network would be good.

[orbeckst]

@xiki-tempula are you going to fix #3259 as part of this PR?

[orbeckst]

@fiona-naughton , what needs to be done to merge by Friday?

[xiki-tempula]

I have resolved the conflicts in case this could be merged today.

[fiona-naughton]

thanks @xiki-tempula - I'll let the checks finish running, but lgtm!