-
Notifications
You must be signed in to change notification settings - Fork 637
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hbond excl fix #3242
Hbond excl fix #3242
Conversation
Hello @richardjgowers! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-04-28 07:32:55 UTC |
Codecov Report
@@ Coverage Diff @@
## master #3242 +/- ##
==========================================
+ Coverage 90.99% 91.84% +0.84%
==========================================
Files 162 167 +5
Lines 22217 22737 +520
Branches 3203 3203
==========================================
+ Hits 20216 20882 +666
- Misses 1378 1771 +393
+ Partials 623 84 -539
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of tests & logic comments.
Also my cython is probably too green to probably review stuff, if someone with more experience can have a look that'd be great - pinging @MDAnalysis/coredevs
pair = capped_distance(self.h.positions, self.a.positions, max_cutoff=self.d_crit, box=box, | ||
return_distances=False) | ||
if not self.exclusions is None: | ||
pair = pair[~ _in2d(pair, self.exclusions)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised there was no suitable numpy operation for this :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The below passes the analysis suite locally without using a custom Cython function. Maybe double check with Richard if his mixture of Cython/C++ is more efficient -- probably any advantage is related to the integer only nature of the data structure?
--- a/package/MDAnalysis/analysis/hbonds/hbond_autocorrel.py
+++ b/package/MDAnalysis/analysis/hbonds/hbond_autocorrel.py
@@ -210,6 +210,7 @@ from six import raise_from
import numpy as np
import scipy.optimize
+from scipy.spatial.distance import cdist
import warnings
@@ -433,7 +434,7 @@ class HydrogenBondAutoCorrel(object):
pair = capped_distance(self.h.positions, self.a.positions, max_cutoff=self.d_crit, box=box,
return_distances=False)
if not self.exclusions is None:
- pair = pair[~ _in2d(pair, self.exclusions)]
+ pair = pair[~ (cdist(pair, self.exclusions)==0).any(axis=1)]
hidx, aidx = np.transpose(pair)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use cdist but it's odd to use an O(n2) solution when you can just use a set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong view on it--if this is genuinely a performance-critical part of the code maybe the custom maintenance burden is worth it.
Forgot to mention, as far as I understand the old tests weren't picking up that this code path wasn't working. If that's the case, are we sure they are now working properly? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tests. For some reason codecov isn't recognising the ValueError
test, just proposing a match to make sure it's actually picking up the right thing.
pair = capped_distance(self.h.positions, self.a.positions, max_cutoff=self.d_crit, box=box, | ||
return_distances=False) | ||
if not self.exclusions is None: | ||
pair = pair[~ _in2d(pair, self.exclusions)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The below passes the analysis suite locally without using a custom Cython function. Maybe double check with Richard if his mixture of Cython/C++ is more efficient -- probably any advantage is related to the integer only nature of the data structure?
--- a/package/MDAnalysis/analysis/hbonds/hbond_autocorrel.py
+++ b/package/MDAnalysis/analysis/hbonds/hbond_autocorrel.py
@@ -210,6 +210,7 @@ from six import raise_from
import numpy as np
import scipy.optimize
+from scipy.spatial.distance import cdist
import warnings
@@ -433,7 +434,7 @@ class HydrogenBondAutoCorrel(object):
pair = capped_distance(self.h.positions, self.a.positions, max_cutoff=self.d_crit, box=box,
return_distances=False)
if not self.exclusions is None:
- pair = pair[~ _in2d(pair, self.exclusions)]
+ pair = pair[~ (cdist(pair, self.exclusions)==0).any(axis=1)]
hidx, aidx = np.transpose(pair)
|
||
for i in range(arr2.shape[0]): | ||
p = pair[np.intp_t, np.intp_t](arr2[i, 0], arr2[i, 1]) | ||
hits.insert(p) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we need a comment or two about the C++ magic happening here; I suppose we are slowly growing expertise with devs familiar with C++ (I'm not one of them).
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
WRT codecov, I'm guessing Cython linenumbers and linetracing isn't a match made in heaven. |
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Fixes #2987
Changes made in this Pull Request:
PR Checklist