search

cesmix-mit / InteratomicPotentials.jl

Contains methods and types for a variety interatomic potentials.
MIT License
27 stars 6 forks source link

fix new distance calculation for Dirichlet boundary conditions #16

Closed jrdegreeff closed 2 years ago

jrdegreeff commented 2 years ago

These changes to the sign of the distance have uncovered a bug in the SNAP code that assumed the old sign (hence the tests not passing). I have verified that doing yi - xi is incorrect for the LJ implementation (particles are attracted to each other way too much and accelerations blow up). I don't know anything about the SNAP code, so I don't want to go poking around changing signs, but I'd suggest that we fix that issue and get the tests passing again in a second commit before merging this PR.

dallasfoster commented 2 years ago

These changes to the sign of the distance have uncovered a bug in the SNAP code that assumed the old sign (hence the tests not passing). I have verified that doing yi - xi is incorrect for the LJ implementation (particles are attracted to each other way too much and accelerations blow up). I don't know anything about the SNAP code, so I don't want to go poking around changing signs, but I'd suggest that we fix that issue and get the tests passing again in a second commit before merging this PR.

In general the force between atom_i and atomj is calculated using r{ij}, that is the vectors with tail at atom_i and head at atom_j. In reference to the code, that means we need to return yi - xi since (essentially) yi = atom_j and xi = atomi. The reason that the SNAP tests fail is because I am verifying them against LAMMPS results, which calculates r{ij} as atom_j - atomi. NBodySimulator uses r{ij} = atom_i - atom_j.

jrdegreeff commented 2 years ago

These changes to the sign of the distance have uncovered a bug in the SNAP code that assumed the old sign (hence the tests not passing). I have verified that doing yi - xi is incorrect for the LJ implementation (particles are attracted to each other way too much and accelerations blow up). I don't know anything about the SNAP code, so I don't want to go poking around changing signs, but I'd suggest that we fix that issue and get the tests passing again in a second commit before merging this PR.

In general the force between atom_i and atomj is calculated using r{ij}, that is the vectors with tail at atom_i and head at atom_j. In reference to the code, that means we need to return yi - xi since (essentially) yi = atom_j and xi = atomi. The reason that the SNAP tests fail is because I am verifying them against LAMMPS results, which calculates r{ij} as atom_j - atomi. NBodySimulator uses r{ij} = atom_i - atom_j.

The displacement vectors are all internal to InteratomicPotentials, so what NBodySimulator does should be irrelevant. Basically one way or another, the sign of the displacement needs to be consistent with the sign convention used in the force calculation. The forces are the only thing that are eventually passed to NBS via Atomistic. We can certainly stick with the convention that is accordance with LAMMPS, but I think this is indicative of a bug somewhere else in the force calculations.