Compute bond without using minimum image convention

[clpetix]

Compute bond without using minimum image convention to avoid issues with bonds that are longer than L/2.

[clpetix]

@mphoward, I've copied the file over here and made a change I think should get the unwrapped positions to compute the bond length.

I'm fairly unfamiliar working with C++ projects. Could you explain what the process is like for getting these changes connected in and usable in HOOMD?

[mphoward]

Thanks! See comment for how to implement the change.

The next step will be to add enough code to make this template class compile.

1. Make a file export_ImageBondPotentialHarmonic.cc, and explicitly wrap the template of the export function from the end of this file for EvaluatorBondHarmonic from hoomd::md. Something like:

   void export_ImageBondPotentialHarmonic(pybind11::module& m)
       {
       hoomd::azplugins::detail::export_ImageBondPotential<hoomd::md::EvaluatorBondHarmonic>(m, "ImageBondPotentialHarmonic");
       }
   

   with necessary includes.

2. Add this file to src/CMakeLists.txt with the other cc files.

3. Call the export function in module.cc in the bond potential section.

[mphoward]

We will end up simplifying this file using subclassing later, it's fine for now though.

[clpetix]

@mphoward, I've applied those changes and was able to get it to compile without error after a few small tweaks.

[mphoward]

Looking good! After applying these changes, the next step will be to add a Python object for this class to bond.py and test it. A good name might be like ImageHarmonic or something like that.

You should be able to just copy one of the existing classes and change the Python / C++ class names. I am OK if you also want to farm out most of the documentation to HOOMD as well, since it is the same potential other than how the bond distance is calculated. That part should be documented clearly.

For testing, you can start by adding this potential to the standard list that is there. Then, we should also add a new test case that checks for Image bond potentials that it is computed correctly even in a small box (e.g., put them on opposite edges but the same image, so the true distance is longer than the minimum distance).

[clpetix]

@mphoward, I have this working on 1 CPU core and added the two test scenarios we discussed. It has an issue when running under MPI. I'm not sure what is happening there. Let me know if you have any ideas on what that could be, before we start with GPU & reducing code duplication.

[mphoward]

Try requesting that the image be communicated with ghost particles to see if that fixes the problem you were having in MPI. There are still going to be issues in MPI simulations if bond lengths get too long, but that cannot be fixed. We should just document that and make sure the code would throw an error (it probably does) already.

[clpetix]

@mphoward, the parametrized tests are working fine without MPI, but something is off with the MPI versions. The MPI tests worked fine here using the GSD package so I think it must be something with using hoomd.snapshot.

Could you have a look and see if anything sticks out? I think I must just be overlooking something

[mphoward]

The tests are deadlocking, but it isn't obvious to me where it is happening since there isn't any output. Are you able to compile with MPI locally and run them to do debugging?

[clpetix]

@mphoward, the local version was also gridlocking with no output, but I was able to go line by line, raising errors to figure where it was locking. I had:

if sim.device.communicator.rank == 0:
            numpy.testing.assert_array_almost_equal(
                harmonic.forces, expected_forces, decimal=4
            )

It should have been:

forces = harmonic.forces
if sim.device.communicator.rank == 0:
          numpy.testing.assert_array_almost_equal(forces, expected_forces, decimal=4)

One day I'll figure out this MPI stuff 😅

[mphoward]

Ah yes, that is expected. That property returns the forces on all particles, even if they are distributed onto multiple ranks, so it is a collective call that needs to be invoked on all processors. The data is only valid on rank 0 though.

[mphoward]

Here are suggested changes to switch to using inheritance from PotentialBond!

[clpetix]

@mphoward, thank you for the help switching this over to use inheritance. I think I have it all working!

[mphoward]

Looks great! Here are just a couple small things. We'll discuss the GPU implementation next.

[clpetix]

@mphoward, I have the GPU implementation working locally and all the tests are passing. I'm not sure what is happening with the actions here.

This should be good for you to have another look at, when you have time!

[mphoward]

I'm not sure what is happening with the actions here.

There was some authentication error preventing the runner from pulling the Docker container it needs. I re-ran the workflow, and whatever was causing that problem seems to have resolved.

[mphoward]

Thanks, this looks great! Can you please take care of these comments, then we can ping Antonia to have a look.

[astatt]

Since all of Mike's comments are resolved and I did not find any additional issues, I think this is ready to merge. Thank you!