Skip to content
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

Neighbor search with nsgrid.FastNS yields different pairs for swapped coords and search_coords #2229

Open
ezavod opened this issue Mar 30, 2019 · 7 comments

Comments

@ezavod
Copy link

commented Mar 30, 2019

Expected behavior

Let's say one has A and B particles. It should make no difference if one constructs FastNS with coordinates of A and searches with coordinates of B or vice versa. The pairs of A-B that are found within a certain cutoff should be invariant against exchanging A around B or B around A.

Actual behavior

In some cases one gets different pairs depending on the order.

Code to reproduce the behavior

This is simple.gro (notice the extra 5 in the last decimal place of the first box vector):

Written by MDAnalysis
   2
    1RA       A    1   0.000   0.000   0.000
    2RB       B    2   5.500   0.000   0.000
   5.745585  5.00000   5.00000

This script searches pairs of A-B within a cutoff of 3. With PBC A and B are well within this distance.

import MDAnalysis
from MDAnalysis.lib.nsgrid import FastNS

u = MDAnalysis.Universe("simple.gro")

max_cutoff = 3
configuration = u.select_atoms("name A").positions
reference = u.select_atoms("name B").positions
box = u.trajectory[0].dimensions

gridsearch = FastNS(max_cutoff, configuration, box=box)
results = gridsearch.search(reference)
pairs = results.get_pairs()
print("Constructed with A; searched with B:", pairs)

gridsearch = FastNS(max_cutoff, reference, box=box)
results = gridsearch.search(configuration)
pairs = results.get_pairs()
print("Constructed with B; searched with A:", pairs)

Output:

Constructed with A; searched with B: [[0 0]]
Constructed with B; searched with A: []

If one now removes the last decimal place of the first box vector every thing works as expected. One finds the pair no matter which order. So the bug only happens under special circumstances.

Current version of MDAnalysis

  • MDAnalysis version 0.19.2
  • Python version 3.7.2
  • openSUSE Tumbleweed 64bit

I also played around with the current develop branch (9428866) but this problem is still persistent. However one has to adjust the box size slightly, probably because of the extra margin in #2136.

Impact
I noticed this bug while using around selections. AroundSelection, SphericalLayerSelection, SphericalZoneSelection and PointSelection in core/selection.pyall use capped_distance. In there a suitable search method is chosen, which is nsgrid for all but small systems. Therefore I suspect that all geometric selections have this problem for typical system sizes.
How often this happens depends of cause on the number of particles near the box boundary. For my systems in ~10% of frames a wrong number of neighbors was found.

Edit
This also occurs without PBC. This is simple_no_pbc.gro:

Written by MDAnalysis
    2
    1RA       A    1   0.000   0.000   2.929
    2RB       B    2   0.000   0.000   2.823
   4.50000   5.50000  10.98375

Using the script from above this seems to work, but if one changes the z coordinates a bit,
i.e. by inserting configuration[0][2] += 1e-6 before the first search,
it does not work anymore. It does not matter if one uses pbc=False explicitly in the constructor. The x, y coordinates of the particles have seemingly no influence, however altering the x,y box vectors changes the behavior. Small changes in any z component also changes the outcome.
This happens in ~1/250 frames for my systems anyway.

@orbeckst

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

@orbeckst

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

@ezavod did you try the same with the development branch?

@ayushsuhane

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

@ezavod Thanks for posting the bug. It seems like it is one of those problematic edge cases. Although I must agree the behavior is strange.

I am not sure at this point but I suspect the problem is in one of the floating point operations/ floating to integer conversion somewhere in nsgrid.pyx. @orbeckst I will try to get to the root of the problem in a couple of days.

Meanwhile, if someone wants to fix the problem, I would suggest checking if the bug resides in capped_distances or nsgrid. One way of finding it could be to pass different arguments (method='bruteforce, 'pkdtree') in capped_distance, and check if it gives the correct solution.

@ezavod

This comment has been minimized.

Copy link
Author

commented Apr 2, 2019

@orbeckst yes, both errors also occur with latest development branch (9428866).
@ayushsuhane I also tested capped_distance with all three methods. pkdtree and bruteforce work both without errors for my trajectories.
I agree that this might be somewhere in nsgrid.pyx. I don't had the time to fully understand whats going on there, but coord2cellidseems to return different indices.

Edit: clicked wrong button and accidentally closed the issue

@orbeckst

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

@ayushsuhane @richardjgowers @jbarnoud @NinadBhat @tylerjereddy do we have sufficiently extensive tests for capped_distances()? Can we use hypothesis to build some smarter tests? This might also help to gain a bit more insights into where the capped_distances() machinery might be failing.

As pointed out by @ezavod we now use capped_distances() extensively and we are happy about the speed-ups but they cannot come at the cost of correctness.

@orbeckst

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

Also cc @zemanj

@ezavod

This comment has been minimized.

Copy link
Author

commented Jun 13, 2019

It might be a good start for testing to compare the results of capped_distances for the various (bruteforce, pkdtree, nsgrid) methods against each other. The implementation between them is quite different so this might catch some major problems. However no substitute for rigorous testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.