added Cython PBC

[richardjgowers]

So in #1996 @seb-buch introduced some code (originally from gromacs) which featured a different way of doing PBC for triclinic boxes. Our current method searches all neighbouring images to find the closest, the gromacs approach builds a list of images to search (based on box geometry) and then only searches these if required.

I went ahead and rewrote it a little to work for all cases, I think it's an improvement over our current solution of C header + Cython.

How to read this PR:

• new_distances.pyx shows how distance_array looks with the new PBC function
• pbc.pyx is mostly lifted and trimmed from Gromacs. It defines a PBC object (box with extra info attached) and a minimum_image function which expects the PBC and a separation

Pros:

• Cython reads much nicer than C
• gets rid of all our different functions (we currently have 3 C functions for each function in lib.distances
• faster (about 4x faster for triclinic boxes, orthogonal performance the same)

Cons:

• not sure I can get the openmp thing to work again. There is cython.prange though (I think parallelism like pmda is the way to go anyway though)

[kain88-de]

Are you using the shifts that are calculated?

[richardjgowers]

@kain88-de this seems to be using the shifts

[kain88-de]

This part is missing from @seb-buch conversion in the ns-grid code. This is why I'm interested in maxcutoff2. The code has two max cutoff value and I don't see the difference right away.

[richardjgowers]

https://github.com/gromacs/gromacs/blob/7b95d9dd145fcf9141b5aa489c47920a3c2e2cc3/src/gromacs/pbcutil/pbc.cpp#L170

This is where it's taken from. WRT two max cutoffs, is this how it looks at both orthogonal and triclinic box vectors?

[kain88-de]

can you explain what max_cutoff2 is and when this condition is met? I think it will also affect the code from @seb-buch for the ns-grid.

[richardjgowers]

It's only used for triclinic conditions, and I think @seb-buch said that his grid code remaps a triclinic cell to orthogonal conditions, so shouldn't affect that afaik.

This is what I got confused about yesterday, as the tric_vec stuff was removed.

[richardjgowers]

From what I can tell, max cutoff is the max distance before an image might be required. Anything under max cutoff can never be improved upon by shifting around. If the norm of a vector is larger, the algorithm goes through the shift vectors to see if it can improve (lessen) the vector.

…

On Thu, Jul 26, 2018 at 12:25 AM, Max Linke ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In package/MDAnalysis/lib/pbc.pyx <#2007 (comment)> : > + Modifies dx in place + """ + cdef int i, j + cdef float d2min, d2trial + cdef float[3] dx_start, trial + + if pbc.pbc_type == TRICLINIC: + for i in range(2, -1, -1): + while (dx[i] > pbc.hbox_diag[i]): + for j in range(i, -1, -1): + dx[j] -= pbc.box[i][j] + while (dx[i] <= pbc.mhbox_diag[i]): + for j in range(i, -1, -1): + dx[j] += pbc.box[i][j] + d2min = norm2(dx) + if d2min > pbc.max_cutoff2: can you explain what max_cutoff2 is and when this condition is met? I think it will also affect the code from @seb-buch <https://github.com/seb-buch> for the ns-grid. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2007 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI0jB1I9josUTPbOMYVo8Uawg83S0eYeks5uKVLmgaJpZM4VgoM0> .

[codecov]

Codecov Report

Merging #2007 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           develop   #2007   +/-   ##
=======================================
  Coverage     88.5%   88.5%           
=======================================
  Files          143     143           
  Lines        17249   17249           
  Branches      2646    2646           
=======================================
  Hits         15267   15267           
  Misses        1385    1385           
  Partials       597     597

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b50c6f0...1aeabe0. Read the comment docs.

[kain88-de]

not sure I can get the openmp thing to work again. There is cython.prange though (I think parallelism like pmda is the way to go anyway though)

not in such a nice way that we have right now. The openMP gave a nice speed boost though. I still like to have this though if it covers more cases. We should first strive to be correct.

[richardjgowers]

@kain88-de so this method seems to give correct triclinic distances. Did you have a good method for finding difficult cases when you looked at it before? Or just generate random coordinates with smallish angles in the triclinic cell?

[richardjgowers]

Ok thinking about it, I think we can just do the same trick. Cython just spits out some C with openmp sections, we compile whatever Cython spits out twice, once with openmp flags once without.