brucefan1983 / GPUMD

Graphics Processing Units Molecular Dynamics
https://gpumd.org/dev
GNU General Public License v3.0
466 stars 116 forks source link

Question about the `gpu_correct_momentum` function #170

Closed ghost closed 2 years ago

ghost commented 2 years ago

I found that when removing the COM moments in the Langevin thermostat, the total moments of the simulated system is equally divided into number_of_atoms parts and subtracted from the velocities of each atom. https://github.com/brucefan1983/GPUMD/blob/bca0d9db3b3cfc8c2fc9a5280df70a3784608c5f/src/integrate/ensemble_lan.cu#L174 When the system contains only one type of atom, this function will work well. But once the masses of the atoms are not the same, this method may modify the relative velocities between atoms (since the velocities subtracted from each atom are not the same), thus the internal motion of the simulated system will not be the same before and after the removal. So I feel that it would be more reasonable if we could first calculate the COM velocities, and then subtract the three velocities equally from the velocities of each atom.

ghost commented 2 years ago

For example, this is the Implementation in OpenMM:

https://github.com/openmm/openmm/blob/fd13a655ec58421a63ef3a97394f4b019dc51706/platforms/reference/src/ReferenceKernels.cpp#L2928

brucefan1983 commented 2 years ago

You are right, the function zero_linear_momentum in src/main_gpumd/velocity.cu has this problem too. Need to be fixed.

brucefan1983 commented 2 years ago

If you are interested in this, you can fix it, otherwise I can fix it at some point.

ghost commented 2 years ago

I'm sorry about this, but I'm literally a noob in CUDA programming and do not know how to write kernel functions. So maybe I have to trouble you to fix the bug 😢

brucefan1983 commented 2 years ago

I have created a PR #172 to fix this issue. Could you review it?

ghost commented 2 years ago

I think the new code is good enough to merge!

brucefan1983 commented 2 years ago

Thanks!