Replace the distance search of finding hydrogens with a dictionary lookup#2519
Merged
richardjgowers merged 3 commits intoMDAnalysis:developfrom Feb 15, 2020
Merged
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2519 +/- ##
===========================================
- Coverage 90.42% 90.33% -0.09%
===========================================
Files 170 170
Lines 23145 23156 +11
Branches 2988 2991 +3
===========================================
- Hits 20928 20919 -9
- Misses 1620 1634 +14
- Partials 597 603 +6
Continue to review full report at Codecov.
|
xiki-tempula
changed the title
| if atom.name in self.donors: | ||
| self._residue_dict[residue.resname][atom.name].update(self._get_bonded_hydrogens(atom).names) | ||
|
|
||
| def _update_donor_h(self, atom_ix, h_donors, donors_h): |
Member
There was a problem hiding this comment.
this needs a docstring, it's not clear who/what/where atom_ix is
richardjgowers
approved these changes
Feb 15, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes made in this Pull Request:
In the original water bridge analysis implementation, the hydrogen associated with the hydrogen bond donor heavy atom were looked up through a distance search, which is repeated every single frame. This PR attempts to generate the dictionary of hydrogen bond donor heavy atom vs hydrogen pair in the first frame and do a dictionary lookup for the rest of the frames.
This PR should reduce the processing time for all the rest of the frames at the cost of increasing the time for the first frame. This PR should also reduce the computational overhead for turning on the option of
update_selection.Here are the benchmark results.
Combined with #2480, the water bridge analysis is now 4X faster than the original implementation and I felt there is probably little room for performance improvement now.
PR Checklist