Feature minimise vectors #3472
Feature minimise vectors #3472
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 2022-01-11 11:03:08 UTC |
|
|
||
| @cython.boundscheck(False) | ||
| @cython.wraparound(False) | ||
| cdef inline void _minimum_image_orthogonal(cython.floating[:] dx, cython.floating[:] box, cython.floating[:] inverse_box): |
There was a problem hiding this comment.
Our current implementations in calc_distances.h are a weird mixed but mostly single precision. I've rewritten these here using cython fused types (aka templates) to allow both single and double precision
Codecov Report
@@ Coverage Diff @@
## develop MDAnalysis/mdanalysis#3472 +/- ##
===========================================
+ Coverage 93.77% 93.87% +0.09%
===========================================
Files 176 176
Lines 23214 23282 +68
Branches 3302 3302
===========================================
+ Hits 21769 21856 +87
+ Misses 1394 1373 -21
- Partials 51 53 +2
Continue to review full report at Codecov.
|
|
Hooray for |
|
I would have thought someone would have asked for |
|
I noticed and didn’t care ;-). I don’t know if we are consistent throughout.
… On Nov 30, 2021, at 10:04 AM, Richard Gowers ***@***.***> wrote:
I would have thought someone would have asked for minimizing with a z by now. We use American spelling iirc?
|
|
Z please :-) might as well keep, or move towards, one standard. |
|
ok @MDAnalysis/coredevs I think this is ready for review |
| # this technically doesn't alter the vector because of periodic boundaries | ||
| shifted_vec = (vec + (box.T * shift).sum(axis=1)).astype(dtype) | ||
|
|
||
| box2 = mdamath.triclinic_box(*box).astype(dtype) |
There was a problem hiding this comment.
I found this slightly confusing for the ortho case until I ran through it and realised that this eventually returns an ortho box for np.eye(3) through the check_box call. in minimize_vectors
Perhaps a quick note will help for clarity and future readability.
|
|
||
| res = distances.minimize_vectors(shifted_vec, box2) | ||
|
|
||
| assert_almost_equal(res, vec, decimal=5) |
There was a problem hiding this comment.
I think assert_allclose() or one of the _ulp() numpy comparisons is preferred now @tylerjereddy?
There was a problem hiding this comment.
@richardjgowers just this one and then good to go
|
Ok hopefully good to go @hmacdope and @MDAnalysis/coredevs |
Towards MDAnalysis/waterdynamics#19 and #3471
Changes made in this Pull Request:
PR Checklist