Using Self Capped distance function in guess_bonds#2006
Using Self Capped distance function in guess_bonds#2006richardjgowers merged 5 commits intoMDAnalysis:developfrom
Conversation
|
As you might notice, after evaluating all the pairs in _pkdtree_capped_self, I call calc_distances which is not efficient. Should I write another cython function to calculate distance for a single pair say A - B particle or there exists any fast implementation other than calc_distances (calc_bonds). @richardjgowers mentioned that we would want to calculate all the distances rather than evaluating them one by one. |
|
Sorry for such a clutter in diff. There is another PR to replace BioKDtree. I will clean the history once it is merged. |
|
@ayushsuhane yeah there's indices = KDTree.something(coords1, coords2, box)
i, j = indices
# i is array of indices referring to coords2, j is for coords2
# calc_bonds is like zip over the coordinates for distances
dists = calc_bonds(coords1[i], coords2[j], box)
winners = dists < max_cutoff
i, j = i[winners], j[winners] |
package/MDAnalysis/lib/distances.py
Outdated
| def capped_distance(reference, configuration, max_cutoff, min_cutoff=None, box=None, method=None): | ||
| """Calculates the pairs and distance within a specified distance | ||
| def capped_distance(reference, configuration, max_cutoff, min_cutoff=None, | ||
| box=None, method=None, equal=False): |
There was a problem hiding this comment.
I don't like this equal keyword. Make a new function called self_capped_distance then it's similar to how self_distance_array looks
There was a problem hiding this comment.
This would result in repeating most of the code again especially the _determine_method function.
Codecov Report
@@ Coverage Diff @@
## develop #2006 +/- ##
===========================================
+ Coverage 88.45% 88.51% +0.06%
===========================================
Files 143 143
Lines 17223 17273 +50
Branches 2637 2649 +12
===========================================
+ Hits 15234 15290 +56
+ Misses 1389 1385 -4
+ Partials 600 598 -2
Continue to review full report at Codecov.
|
|
@ayushsuhane can be rebased now |
…eforce_capped_self, pkdtree_capped_self + tests
|
@richardjgowers Are any changes required here. While most of the code is repeatable inorder to have same signature to capped_distance, is it something which is desired. WRT determine method, I will redo the benchmarks with nsgrid and change it in #2008 . Apparently, guess_bonds is already present in the benchmarks, I will have to check it as well and see how the performance is changing across commits. I checked with |
richardjgowers
left a comment
richardjgowers
left a comment
There was a problem hiding this comment.
Yeah looks good, few comments and get the tests green and we can merge
|
|
||
| Enhancements | ||
|
|
||
| * Added lib.distances.self_capped_distance to internally select the |
package/MDAnalysis/lib/distances.py
Outdated
| ----- | ||
| Currently only supports brute force and Periodic KDtree | ||
|
|
||
| .. SeeAlso:: :func:'MDAnalysis.lib.distances.distance_array' |
| else: | ||
| idx = np.where((dist < max_cutoff))[0] | ||
| for other_idx in idx: | ||
| j = other_idx + 1 + i |
There was a problem hiding this comment.
Add a comment for this, not obvious at first
|
@richardjgowers please review whenever you get the time |
2 participants
guess_bonds function uses brute force which becomes really slow very rapidly with increase in number of atoms. Additional function over capped _distance specifically to handle two equal arrays are introduced and the guess_bonds function is modified
PR Checklist