m3g / CellListMap.jl

Flexible implementation of cell lists to map the calculations of particle-pair dependent functions, such as forces, energies, neighbor lists, etc.
https://m3g.github.io/CellListMap.jl/
MIT License
87 stars 4 forks source link

update!() fails with disperse coordinate points #79

Closed cdelv closed 1 year ago

cdelv commented 1 year ago

Hi, I have been using CellListMap for a while, and a few days ago updated the package. To my surprise, now disperse systems as simple as 1 or 2 coordinates throw an error that didn't happen before. Here is son simple code that can reproduce the error:

r = [[1.0,1.0,1.0]]
system = InPlaceNeighborList(x=r, cutoff=3.0, parallel=false)
list = neighborlist!(system)
update!(system, r)

This also happens when data is a bit dispersed. For instance, with these coordinates it happens too: r = [[1.0,1.0,1.0], [10.0, 1.0, 1.0], [3.0, 1.0, 1.0]]

The error is the following:

UNIT CELL CHECK FAILED: distance between cell planes too small relative to cutoff.
ArgumentError:  Unit cell matrix does not satisfy required conditions.

It is weird because list = neighborlist!(system) is working fine.

For now, I will revert to the old version, but this is something that should be fixed.

Best regards.

lmiq commented 1 year ago

Thanks for reporting. I'll fix that asap.

lmiq commented 1 year ago

It is fixed in version 0.8.13 to appear in the registry as soon as it is merged. Sorry for that, version 0.8.12 was a significant rewrite of the package and that slipped off. I've added now additional unit tests to avoid that regression again.

cdelv commented 1 year ago

Thank you for your quick fix, although I regret to say that the problem still persists. For example this code:

r = [[7,10,10], [18,10,10]]
system = InPlaceNeighborList(x=r, cutoff=3.0, parallel=false)
list = neighborlist!(system)
r = [[10.89658911843461, 3.709237933444153, 10.0], [13.894156281793144, 11.054172259416013, 10.0]]
update!(system, r)

will produce the same error.

Im using the 0.8.13 version. As a reference, the 0.8.10 version worked fine with this same code.

Quick edit: It may be helpful to allow the user to define the expected bounds of the simulation for nonperiodic systems to avoid these unexpected errors?

Best regards.

lmiq commented 1 year ago

The fix there is easy. I'm curious however on how are you arriving to these situations. I have quite an extensive test test of pathological coordinates, and none of my tests were failing. Maybe that can be useful for adding additional tests here. (The issue is related to testing "greater or equal" conditions in the limit of the numerical accuracy of floating point operations. Now I added a safer boundary way larger than that accuracy for the dimensions where the problem were occurring).

I will release version 0.8.14 with the new fix as soon as the CI tests pass. If you want you can check it by using the release-0.8.14 dev branch.

lmiq commented 1 year ago

Fixed in 0.8.14 - released now in the registry.

Please let me (always) know if you experience this or any other issue.

cdelv commented 1 year ago

Hi @lmiq, great to hear that it's fixed.

I'm using CellListMap for contact detection in a DEM code I'm developing for granular materials simulations. I have a lot of sample simulations and was re-running all of them, and the error just appeared. That's how I found the pathological coordinates. My simulations usually run inside a big box, some have only 1 or 2 particles, and some have a lot of particles and are very crowded.

I updated the package and re-ran all my simulations, and everything appears to be working fine.

Again, thank you very much for all your work. Your package is great.

Best regards.