Periodic improper support #1662

kierannp · 2023-11-26T19:32:57Z

Description

Add support for a periodic improper force for 4 atoms bonded as an improper https://manual.gromacs.org/current/reference-manual/functions/bonded-interactions.html#proper-dihedrals-periodic-type

Motivation and context

This is a common functional form in CHARMM-like forcefield and is an easy extension of the periodic dihedral already supported.

How has this been tested?

Created python functions for calculating the force and energy resulting from this functional formal, see hoomd.md.pytest.test_improper.py for detailed test cases and functions to recreate test results.

Change log

Added test and support for periodic impropers

Checklist:

I have reviewed the Contributor Guidelines.
I agree with the terms of the HOOMD-blue Contributor Agreement.
My name is on the list of contributors (sphinx-doc/credits.rst) in the pull request source branch.

CalCraven · 2023-11-26T20:11:51Z

One thing to note about this PR is that the support for periodic impropers is important for running GAFF forcefields in HOOMD, which is recently supported in other engines via MoSDeF. One thing to note while @kierannp and I were working on this is the naming in HOOMD, which can be a bit confusing.
HOOMD has a periodic dihedral support already, which is called via hoomd.md.dihedral.Periodic. But the cpp code that is references is name [HarmonicDihedralForceCompute.cc(https://github.com/glotzerlab/hoomd-blue/blob/trunk-patch/hoomd/md/HarmonicDihedralForceCompute.cc). But again, it is a periodic dihedral force.

There already is a HarmonicImproperForceCompute.cc that computes a true a hoomd.md.improper.Harmonic functional form, and we are adding a hoomd.md.improper.Periodic. So we will call the cpp files PeriodicImproperForceCompute.cc. We may benefit at this time to rename the HarmonicDihedralForceCompute.cc (and like files) to PeriodicDihedralForceCompute.cc to reduce future confusion and potentially open the door for a true HarmonicDihedral force, which should look like any other harmonic force with an (a-a0)**2 type syntax, despite how LAMMPS notates it with a cosine function, and uses the term quadratic to denote harmonic type dihedrals to confuse all of us.

joaander

This looks like a good start! Here are some suggestions for improvement.

hoomd/md/PeriodicImproperForceCompute.h

hoomd/md/improper.py

joaander · 2023-11-27T18:49:44Z

hoomd/md/PeriodicImproperForceCompute.cc

+        assert(improper.tag[3] <= m_pdata->getMaximumTag());
+
+        // transform a, b, and c into indices into the particle data arrays
+        // MEM TRANSFER: 6 ints


Suggested change

// MEM TRANSFER: 6 ints

Reminder to remove this line.

joaander · 2023-11-27T18:53:12Z

hoomd/md/improper.py

@@ -110,3 +110,46 @@ def __init__(self):
                chi0=hoomd.data.typeconverter.nonnegative_real,
                len_keys=1))
        self._add_typeparam(params)
+
+class Periodic(Improper):


Add this class name to the list in module-md-improper.rst so the documentation appears.

hoomd/md/pytest/test_improper.py

joaander · 2023-11-27T19:04:46Z

There already is a HarmonicImproperForceCompute.cc that computes a true a hoomd.md.improper.Harmonic functional form, and we are adding a hoomd.md.improper.Periodic. So we will call the cpp files PeriodicImproperForceCompute.cc. We may benefit at this time to rename the HarmonicDihedralForceCompute.cc (and like files) to PeriodicDihedralForceCompute.cc to reduce future confusion and potentially open the door for a true HarmonicDihedral force, which should look like any other harmonic force with an (a-a0)**2 type syntax, despite how LAMMPS notates it with a cosine function, and uses the term quadratic to denote harmonic type dihedrals to confuse all of us.

Yes, feel free to rename HarmonicDihedralForceCompute* to PeriodicDihedralForceCompute* at the C++ level, both with the file and class names. Please make this change in a separate pull request.

…ocation

CalCraven

Looking good so far.

hoomd/md/pytest/test_improper.py

CalCraven

Looking good, I think there are two more of Josh's edits still to get at. Also add your name to this file as well, should we add a note to the changelog?

CalCraven · 2023-11-29T23:04:16Z

hoomd/md/PeriodicImproperForceCompute.cc

+        assert(improper.tag[3] <= m_pdata->getMaximumTag());
+
+        // transform a, b, and c into indices into the particle data arrays
+        // MEM TRANSFER: 6 ints


Reminder to remove this line.

joaander · 2023-11-29T23:18:38Z

should we add a note to the changelog?

Please don't. HOOMD has so many PRs that all the change log entries would conflict. I build the change log at release time from the drafts in each of the PR descriptions.

… periodic improper; made modification for suggestions; altered test case in documentation to not produce error

…throws error/warning

hoomd/md/PeriodicImproperForceGPU.cu

Co-authored-by: Tim Moore <mtimc@umich.edu>

Also complete misc cleanup to bring the code up to modern HOOMD coding standards.

joaander

Thanks for the complete PR and extensive tests.

I pushed a commit that moves the parameters into a GPUArray to reduce code duplication and the chances for errors in that duplication. I also made a few other changes to bring error messages up to modern HOOMD coding standards.

Could you double check my changes and let me know if they are correct? I got a little lost with the parameters. It seems that "d" and "sign" are the same parameter. It also seems that "n", "multi", and "m" are also all the same. You might consider modifying the code to keep the same names throughout to avoid confusion.

kierannp · 2023-12-22T18:51:16Z

Thanks for the complete PR and extensive tests.

I pushed a commit that moves the parameters into a GPUArray to reduce code duplication and the chances for errors in that duplication. I also made a few other changes to bring error messages up to modern HOOMD coding standards.

Could you double check my changes and let me know if they are correct? I got a little lost with the parameters. It seems that "d" and "sign" are the same parameter. It also seems that "n", "multi", and "m" are also all the same. You might consider modifying the code to keep the same names throughout to avoid confusion.

The changes you made appear correct. I tried to use the same naming conventions as in similar scripts in HOOMD for calculating forces. I will look into making these variables names more consistent and explicit.

CalCraven · 2023-12-23T14:54:45Z

It seems that "d" and "sign" are the same parameter. It also seems that "n", "multi", and "m" are also all the same.

I tried to use the same naming conventions as in similar scripts in HOOMD for calculating forces.

To clarify a bit more, d is named sign because d is always +-1. n is the multiplicity of the cos function. I would second the fact that we should stick to one value within hoomd. I believe these were derived from this line. When I pull a PR to rename the HarmonicDihedralForce to PeriodicDihedralForce, we can make the same consistent name changes to those files as well.

kierannp added 6 commits October 14, 2023 12:00

Added notebook and basic improper tests talked about

909b89c

Added copied improper force calculator files

f07ac51

Added Periodic improper class in python

37e9a8e

Added dummy files

dc3412e

Added periodic improper test cases

d5ed605

Added python functions to generate test values

69fb3ae

kierannp requested review from a team as code owners November 26, 2023 19:32

kierannp requested review from tcmoore3, b-butler and mariaward10 and removed request for a team November 26, 2023 19:32

Added cpp periodic dihedral

c269a48

Periodic Improper test cases passing and compilation for CPU working

3e6eabb

joaander reviewed Nov 27, 2023

View reviewed changes

kierannp added 3 commits November 27, 2023 15:59

Added suggested changes except GPUArray due to issues with memory all…

930501a

…ocation

Corrected formatting

091dd6d

correct the corrected formatting

04711e0

CalCraven suggested changes Nov 28, 2023

View reviewed changes

hoomd/md/pytest/test_improper.py Outdated Show resolved Hide resolved

kierannp added 5 commits November 29, 2023 11:31

Finished modifications suggest in PR review

7992423

Improper documentation

e8a9547

Separate file for improper test case generation

ae5c63e

Corrected formatting and documentation

30e7464

Put generate function back into test file

b90d0d9

kierannp requested a review from CalCraven November 29, 2023 20:32

CalCraven suggested changes Nov 29, 2023

View reviewed changes

kierannp added 3 commits December 2, 2023 15:08

Created wrapper class for periodic improper math; add GPU support for…

8649924

… periodic improper; made modification for suggestions; altered test case in documentation to not produce error

Corrected harmonic to periodic

54ad3a4

Changed documentation example to match harmonic example otherwise it …

c76cf01

…throws error/warning

joaander added the validate Execute long running validation tests on pull requests label Dec 6, 2023

kierannp requested a review from joaander December 6, 2023 19:14

Added names to list of contributers

9ec1ddd

joaander changed the base branch from trunk-patch to trunk-minor December 14, 2023 13:08

tcmoore3 approved these changes Dec 19, 2023

View reviewed changes

hoomd/md/PeriodicImproperForceGPU.cu Outdated Show resolved Hide resolved

joaander and others added 2 commits December 21, 2023 09:05

Update hoomd/md/PeriodicImproperForceGPU.cu

a37891c

Co-authored-by: Tim Moore <mtimc@umich.edu>

Use periodic_improper_params in the base and GPU classes.

adfcb74

Also complete misc cleanup to bring the code up to modern HOOMD coding standards.

joaander approved these changes Dec 21, 2023

View reviewed changes

Run clang-format.

3a01e18

changed variable names to single format

a6db5d9

joaander merged commit 5ea77f0 into glotzerlab:trunk-minor Jan 11, 2024
40 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Periodic improper support #1662

Periodic improper support #1662

kierannp commented Nov 26, 2023 •

edited

CalCraven commented Nov 26, 2023

joaander left a comment

joaander Nov 27, 2023

CalCraven Nov 29, 2023

joaander Nov 27, 2023

joaander commented Nov 27, 2023

CalCraven left a comment

CalCraven left a comment

CalCraven Nov 29, 2023

joaander commented Nov 29, 2023

joaander left a comment

kierannp commented Dec 22, 2023 •

edited

CalCraven commented Dec 23, 2023

Periodic improper support #1662

Periodic improper support #1662

Conversation

kierannp commented Nov 26, 2023 • edited

Description

Motivation and context

How has this been tested?

Change log

Checklist:

CalCraven commented Nov 26, 2023

joaander left a comment

Choose a reason for hiding this comment

joaander Nov 27, 2023

Choose a reason for hiding this comment

CalCraven Nov 29, 2023

Choose a reason for hiding this comment

joaander Nov 27, 2023

Choose a reason for hiding this comment

joaander commented Nov 27, 2023

CalCraven left a comment

Choose a reason for hiding this comment

CalCraven left a comment

Choose a reason for hiding this comment

CalCraven Nov 29, 2023

Choose a reason for hiding this comment

joaander commented Nov 29, 2023

joaander left a comment

Choose a reason for hiding this comment

kierannp commented Dec 22, 2023 • edited

CalCraven commented Dec 23, 2023

kierannp commented Nov 26, 2023 •

edited

kierannp commented Dec 22, 2023 •

edited