Ensure 'HydrogenBondAnalysis' returns the correct distances when using the 'between' keyword #4092
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #4092 +/- ##
===========================================
- Coverage 93.59% 91.94% -1.65%
===========================================
Files 192 192
Lines 25134 25138 +4
Branches 4056 4051 -5
===========================================
- Hits 23524 23114 -410
- Misses 1092 1526 +434
+ Partials 518 498 -20
☔ View full report in Codecov by Sentry. |
p-j-smith
force-pushed
the
fix/hbonds-between
branch
from
e8a9d93 to
2516c04
Compare
| if self.between_ags is not None: | ||
| tmp_donors, tmp_hydrogens, tmp_acceptors = \ | ||
| self._filter_atoms(tmp_donors, tmp_hydrogens, tmp_acceptors) | ||
| between_mask = self._filter_atoms(tmp_donors, tmp_acceptors) | ||
| tmp_donors = tmp_donors[between_mask] | ||
| tmp_hydrogens = tmp_hydrogens[between_mask] | ||
| tmp_acceptors = tmp_acceptors[between_mask] | ||
| d_a_distances = d_a_distances[between_mask] |
There was a problem hiding this comment.
a little out of scope, but would it make more sense to filter before the call to capped_distances?
There was a problem hiding this comment.
good point - that would definitely be better, but it's probably not a straightforward change and I imagine there are lots of ways to do it. One way would be to iterate over the pairs of atom selections passed to between and calculate donor-acceptor distances for each pair. So e.g. if between=[["protein", "SOL"], ["protein", "protein"]], we would calculate:
- protein donor to SOL acceptor distances
- protein acceptor to SOL donor distances
- protein donor to protein acceptor distances
and concatenate them. But maybe that should be a separate pr after this fix is in?
There was a problem hiding this comment.
Separate PR is better — I'd prefer fixing things and knowing that it works and then going for optimizing. I'd be more than happy if someone raises an issue for a performance improvement with a link to this discussion.
There was a problem hiding this comment.
I raised #4130 — please comment there if you have any follow-ups.
|
@richardjgowers can you assign the PR to yourself to shepherd it to completion or otherwise find someone to do it? This seems straightforward enough (at least the original fix) so I wouldn't want the patch to sit here forever, just because it slipped through. Thanks! |
p-j-smith
force-pushed
the
fix/hbonds-between
branch
from
2516c04 to
c3607fd
Compare
Linter Bot Results:Hi @p-j-smith! Thanks for making this PR. We linted your code and found the following: There are currently no issues detected! 🎉 |
There was a problem hiding this comment.
Thanks for the fix @p-j-smith , sorry it took such a long time to review.
Fixes #4091
Changes made in this Pull Request:
betweenkeyword is used inHydrogenBondAnalysis, filter the donor-acceptor distance array to remove hydrogen bonds formed by atoms not in thebetweenselectionbetweenPreviously, the donor, hydrogen, and acceptor atom groups were filtered inside the
HydrogenBondAnalysis._filter_atomsmethod. I've changed the_filter_atomsmethod to instead return a mask that can be used for filtering the atom groups as well as the distance array. This seemed nicer than having to also pass in the distances to_filter_atoms, but let me know if you'd prefer it done differently.PR Checklist
📚 Documentation preview 📚: https://readthedocs-preview--4092.org.readthedocs.build/en/4092/