Parallel version of the new mdanalysis hbond_analysis function#113
Parallel version of the new mdanalysis hbond_analysis function#113orbeckst merged 18 commits intoMDAnalysis:masterfrom
hbond_analysis function#113Conversation
Codecov Report
@@ Coverage Diff @@
## master #113 +/- ##
==========================================
+ Coverage 97.81% 98.19% +0.37%
==========================================
Files 11 12 +1
Lines 642 776 +134
Branches 78 91 +13
==========================================
+ Hits 628 762 +134
Misses 8 8
Partials 6 6
Continue to review full report at Codecov.
|
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
Add entry to CHANGELOG (with issue number)
Otherwise looking good!
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
overall lgtm, see minor comments
CHANGELOG
Outdated
| MM/DD/YYYY VOD555 | ||
|
|
||
| * 0.3.1 | ||
| * 0.3.1 |
There was a problem hiding this comment.
This has to be 0.4.0 because this is a new feature and we do https://semver.org
| # If donors_sel is not provided, use topology to find d-h pairs | ||
| if not self.donors_sel: | ||
|
|
||
| if not (hasattr(u, 'bonds') and len(u.bonds) != 0): |
There was a problem hiding this comment.
Use len(u.bonds) > 0 – will never be negative.
pmda/hbond_analysis.py
Outdated
| if not self.donors_sel: | ||
|
|
||
| if not (hasattr(u, 'bonds') and len(u.bonds) != 0): | ||
| raise Exception( |
There was a problem hiding this comment.
Raise a ValueError instead of generic exception.
Is there a kwarg that could be set when instantiating the class? "set HydrogenBondAnalysis.donors_sel" does not really tell me what to do. This should be clearer – maybe add an example to the docs and include a link to the docs??
|
Sorry – just saw MDAnalysis/mdanalysis#2396 |
| hbonds.hydrogens_sel = f"({protein_hydrogens_sel}) or | ||
| ({water_hydrogens_sel} and around 10 not resname TIP3})" | ||
| hbonds.acceptors_sel = f"({protein_acceptors_sel}) or | ||
| ({water_acceptors_sel} and around 10 not resname TIP3})" |
There was a problem hiding this comment.
@orbeckst Here is the example to set the HydrogenBondAnalysis.donors_sel. The current online MDAnalysis docs is still version 0.20.1, which doesn't contain the new HBond analysis module, so there's no link can be referred to.
There was a problem hiding this comment.
Ok thanks.
Btw, the devdocs are at https://www.mdanalysis.org/mdanalysis/ .
2 participants
Fixes #95
Changes made in this Pull Request:
hbond_analysiisfunction in MDAnalsysis.PR Checklist