Support for AMBER proper and improper dihedrals in write_lammpsdata #896

rwsmith7531 · 2021-05-04T19:07:04Z

PR Summary:

This PR allows AMBER-style improper dihedrals to be written to LAMMPS data file. When CHARMM-style dihedrals are used in a non-CHARMM force field, such as in AMBER force fields, the dihedral 1-4 weighting parameter is automatically set to zero for all dihedrals. A new optional input argument was added to write_lammpsdata to designate when the CHARMM dihedral style is used in a non-CHARMM force field.

Closes #866

PR Checklist

Includes appropriate unit test(s)
Appropriate docstring(s) are added/updated
Code is (approximately) PEP8 compliant
Issue(s) raised/addressed?

…HARMM forcefields.

…ckwards compatibility.

mbuild/formats/lammpsdata.py

for more information, see https://pre-commit.ci

mbuild/tests/test_lammpsdata.py

codecov · 2021-05-05T17:21:40Z

Codecov Report

Merging #896 (b1f96bc) into master (c00fdc0) will increase coverage by 0.22%.
The diff coverage is 92.10%.

@@            Coverage Diff             @@
##           master     #896      +/-   ##
==========================================
+ Coverage   89.45%   89.68%   +0.22%     
==========================================
  Files          63       63              
  Lines        8130     8163      +33     
==========================================
+ Hits         7273     7321      +48     
+ Misses        857      842      -15

Impacted Files	Coverage Δ
mbuild/formats/lammpsdata.py	`92.35% <92.10%> (+4.85%)`	⬆️
mbuild/formats/hoomd_simulation.py	`80.78% <0.00%> (+0.98%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c00fdc0...b1f96bc. Read the comment docs.

…nto lammpsdata_improper

for more information, see https://pre-commit.ci

rmatsum836

Thanks for this much needed addition! Would it also be useful to add tests that check the ordering of atoms in the Dihedral section of the lammps data file?

rmatsum836 · 2021-05-06T19:41:50Z

I've been doing a comparison similar to #899 for amber style impropers in gromacs and LAMMPS. I don't reach agreement between the two engines unless I swap the order of atom 1 and atom 3, which makes me think that the ordering of amber improper atoms from pmd to gromacs may be incorrect.

rmatsum836 · 2021-05-06T21:22:34Z

@rwsmith7531 Would you also mind explaining the conversion from the functional form from Gromacs periodic to Lammps periodic?

rmatsum836 · 2021-05-06T21:57:08Z

Please see the discussion #899. For Amber Impropers, I believe we want to maintain that the central atom is 3rd. Therefore, the central atom should be third when writing out the Amber Impropers to Lammps.

rwsmith7531 · 2021-05-07T19:21:56Z

@rmatsum836 ParmEd does indeed have central-atom third for Amber impropers, but LAMMPS does not take central-atom third in the LAMMPS data file for improper style cvff. Refer to the LAMMPS documentation on improper style cvff here: https://lammps.sandia.gov/doc/improper_cvff.html

rwsmith7531 · 2021-05-07T19:37:30Z

@rmatsum836 The Gromacs periodic style is converted to LAMMPS cvff improper style. The force constant and periodicity are kept the same (just adjusted for different units), while d (in the cvff style) is determined from the dihedral phase. The absolute value of the dihedral phase is only allowed to be 0 (corresponding to d = 1) or 180 (corresponding to d = -1).

rsdefever · 2021-05-07T20:36:41Z

@rmatsum836 ParmEd does indeed have central-atom third for Amber impropers, but LAMMPS does not take central-atom third in the LAMMPS data file for improper style cvff. Refer to the LAMMPS documentation on improper style cvff here: https://lammps.sandia.gov/doc/improper_cvff.html

@rwsmith7531 please take a look at the discussion that was mentioned by @rmatsum836. The fact that LAMMPS specifies central-atom first does not matter in this case. If I'm understanding everything correctly, AMBER impropers need to be specified central atom third no matter what. We did some energy tests, and I also looked at how we write impropers with our MAFFA tools.

I know I was the one who pointed you in the wrong direction on this...so apologies.

bc118 · 2021-05-10T19:05:36Z

So far, I was able to analyze the AMBER impropers and dihedrals. They look correct for the CVFF improper types, according to the LAMMPS website LAMMPS_CVFF_impropers. Note: the CVFF improper types seem to take the central atom 1st from my read.

rsdefever · 2021-05-13T19:48:27Z

@rmatsum836 @rsdefever I see that I should correct the atom order to be central-atom third in the LAMMPS data file, but should the non-central atoms still be sorted by atom type?

@rwsmith7531, yes, I would still sort them. Like I do here in the MCF file.

…edrals.

for more information, see https://pre-commit.ci

mbuild/formats/lammpsdata.py

for more information, see https://pre-commit.ci

…nto lammpsdata_improper

for more information, see https://pre-commit.ci

bc118

Overall this looks like it works well!! Good work!

I ran your test cases and checked the impropers by hand for both CHARMM and AMBER:

If CHARMM they appear to write with the center atom in position 1.
If AMBER they appear to write with the center atom in position 3.

I approve this after my question is addressed for the weighting factor not being 0 or 1 for the CHARMM case.

bc118 · 2021-06-28T17:38:15Z

mbuild/formats/lammpsdata.py

+                if zero_dihedral_weighting_factor:
+                    weight = 0.0
+                else:
+                    weight = 1.0 / len(dihedral.type)


I may have missed it, but should we make a test case that covers when this is not 1 or just set it to 1 if it will never be not 1.?

If anything, we should verify that the assert listed in the tests validate that the weighting factor is being applied like we expect.

@bc118 This PR only really covers support for AMBER force fields, and will maintain the prior (incorrect) determination of the weighting factor for CHARMM force fields because they are not AMBER. The addition of zero_dihedral_weighting_factor ensures that the LAMMPS data writer can handle AMBER proper dihedrals correctly. As this is written, the weighting factor can take values other than 1 in the case of multi-term dihedrals when zero_dihedral_weighting_factor = False.

I intend to follow the suggestion from @justinGilmer by including assertions that the weighting factor is zero when zero_dihedral_weighting_factor = True and the weighting factor is 1 for a single-term dihedral when zero_dihedral_weighting_factor = False (choosing the dihedral such that this behavior should not change when the code is eventually updated to determine CHARMM dihedral weighting factors correctly).

The first assertion was already there and I just added the other one.

justinGilmer

Some small addition to our tests to make sure that the weighting factors are being set to the values we expect. And then it should be good! Thanks @rwsmith7531 !

justinGilmer · 2021-06-30T19:23:40Z

mbuild/formats/lammpsdata.py

+                if zero_dihedral_weighting_factor:
+                    weight = 0.0
+                else:
+                    weight = 1.0 / len(dihedral.type)


If anything, we should verify that the assert listed in the tests validate that the weighting factor is being applied like we expect.

justinGilmer · 2021-06-30T19:27:33Z

mbuild/tests/test_lammpsdata.py

+                found_angles = True
+            elif "Dihedral Coeffs" in line:
+                assert "# charmm" in line
+                assert "#k, n, phi, weight" in out_lammps[i + 1]


For at least the two conditions for the weighting factor we added, can you add in a test that verifies that the weighting factor is being set to zero?

The assertion that the weighting factor is zero when zero_dihedral_weighting_factor = True was already in test_amber. I just added an assertion that the weighting factor is 1.0 in test_save_charmm, where zero_dihedral_weighting_factor is left at the default, which is False.

Unless I’m misunderstanding, if the writhing factor is only 1 or 0, do we need to use the list length? Or do we just set to 1 or 0?

@bc118 When zero_dihedral_weighting_factor = False, the weighting factor is 1.0/(number of terms in the dihedral), which can be 1.0, 0.5, 0.3333, etc.

Should we add a test where it is 1/2, like in a cyclic ring? I’m ok with not doing it if else is.

When the method of determining the weighting factor for CHARMM dihedrals is fixed in a future PR, it should include such a test, but the current method has nothing to do with correcting for rings (which is the actual intended purpose of the weighting factor). Any test made now in which it is 0.5 would probably break when that fix is eventually made.

Ok. Thanks for clarifying. I’m approving then

for more information, see https://pre-commit.ci

…edral." This reverts commit 03156bf.

for more information, see https://pre-commit.ci

bc118

My only comments were clarified and it looks good now.

justinGilmer

My comments/suggestions have been resolved. Thanks for this @rwsmith7531 and all who have reviewed this PR!

…#896) This PR allows AMBER-style improper dihedrals to be written to LAMMPS data file. When CHARMM-style dihedrals are used in a non-CHARMM force field, such as in AMBER force fields, the dihedral 1-4 weighting parameter is automatically set to zero for all dihedrals. A new optional input argument was added to write_lammpsdata to designate when the CHARMM dihedral style is used in a non-CHARMM force field. * Add support for AMBER-style improper torsions. * Apply zero 1-4 weighting parameter in CHARMM-style dihedrals in non-CHARMM forcefields. * Make dihedral weighting parameters not default to zero, to restore backwards compatibility. * Add/improve unit tests and fix bugs found by them. * Update syntax to maintain code style consistency. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Correct mistake in merge conflict correction. * Move imports to tops of functions in unit tests. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Sort non-central atoms in improper dihedral by atom type. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix bug in atom type sorting. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Write central-atom third for AMBER-style impropers. * Add comments explaining multiple reorderings of atoms in improper dihedrals. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Rename borrowed_charmm flag and add additional comment. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Remove atom sorting in improper dihedrals. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Check for the correct dihedral weighting factor in CHARMM dihedral. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Revert "Check for the correct dihedral weighting factor in CHARMM dihedral." This reverts commit 03156bfd2962aeb9e5c8e8bda70c0f1cc91fccbd. * Add a unit test for the weighting parameter of a CHARMM dihedral. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Use get_boundingbox instead of boundingbox. Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

rwsmith7531 added 4 commits April 29, 2021 11:59

Add support for AMBER-style improper torsions.

cc4eb10

Apply zero 1-4 weighting parameter in CHARMM-style dihedrals in non-C…

9d1ba81

…HARMM forcefields.

Make dihedral weighting parameters not default to zero, to restore ba…

c6ced69

…ckwards compatibility.

Add/improve unit tests and fix bugs found by them.

e61d99c

rwsmith7531 requested review from bc118 and rsdefever May 4, 2021 19:10

rsdefever reviewed May 5, 2021

View reviewed changes

mbuild/formats/lammpsdata.py Outdated Show resolved Hide resolved

rwsmith7531 and others added 3 commits May 5, 2021 12:23

Update syntax to maintain code style consistency.

8d44cee

Merge master to resolve merge conflict.

9748085

[pre-commit.ci] auto fixes from pre-commit.com hooks

91445a5

for more information, see https://pre-commit.ci

rsdefever reviewed May 5, 2021

View reviewed changes

mbuild/tests/test_lammpsdata.py Show resolved Hide resolved

rwsmith7531 and others added 6 commits May 5, 2021 13:39

Correct mistake in merge conflict correction.

f2c2b9c

Merge branch 'lammpsdata_improper' of github.com:rwsmith7531/mbuild i…

01996d9

…nto lammpsdata_improper

Move imports to tops of functions in unit tests.

a70a002

[pre-commit.ci] auto fixes from pre-commit.com hooks

cf75b40

for more information, see https://pre-commit.ci

Sort non-central atoms in improper dihedral by atom type.

a5360ef

[pre-commit.ci] auto fixes from pre-commit.com hooks

3aa7706

for more information, see https://pre-commit.ci

rsdefever requested a review from rmatsum836 May 5, 2021 20:44

rwsmith7531 and others added 3 commits May 6, 2021 11:46

Fix bug in atom type sorting.

0dd1404

Integrate pre-commit.ci bot edits and resolve resulting merge conflict.

16771b0

[pre-commit.ci] auto fixes from pre-commit.com hooks

b0019b5

for more information, see https://pre-commit.ci

rmatsum836 requested changes May 6, 2021

View reviewed changes

rwsmith7531 and others added 3 commits May 13, 2021 16:03

Write central-atom third for AMBER-style impropers.

1d4a35e

Add comments explaining multiple reorderings of atoms in improper dih…

1965faf

…edrals.

[pre-commit.ci] auto fixes from pre-commit.com hooks

c41cddd

for more information, see https://pre-commit.ci

rsdefever self-requested a review May 14, 2021 12:35

rsdefever requested changes May 14, 2021

View reviewed changes

mbuild/formats/lammpsdata.py Outdated Show resolved Hide resolved

mbuild/formats/lammpsdata.py Outdated Show resolved Hide resolved

rwsmith7531 and others added 5 commits May 14, 2021 09:50

Rename borrowed_charmm flag and add additional comment.

ef2f803

[pre-commit.ci] auto fixes from pre-commit.com hooks

b102a50

for more information, see https://pre-commit.ci

Remove atom sorting in improper dihedrals.

bb9c162

Merge branch 'lammpsdata_improper' of github.com:rwsmith7531/mbuild i…

d2cdadf

…nto lammpsdata_improper

[pre-commit.ci] auto fixes from pre-commit.com hooks

726f5b0

for more information, see https://pre-commit.ci

justinGilmer requested review from rsdefever, bc118 and rmatsum836 June 25, 2021 21:15

bc118 requested changes Jun 28, 2021

View reviewed changes

justinGilmer requested changes Jun 30, 2021

View reviewed changes

rwsmith7531 and others added 2 commits July 1, 2021 12:23

Check for the correct dihedral weighting factor in CHARMM dihedral.

03156bf

[pre-commit.ci] auto fixes from pre-commit.com hooks

11c62b3

for more information, see https://pre-commit.ci

rwsmith7531 requested review from justinGilmer and bc118 July 1, 2021 16:28

rwsmith7531 and others added 6 commits July 1, 2021 13:30

Revert "Check for the correct dihedral weighting factor in CHARMM dih…

ab2e66c

…edral." This reverts commit 03156bf.

Add a unit test for the weighting parameter of a CHARMM dihedral.

3142394

Merge formatting changes

aabf6a3

[pre-commit.ci] auto fixes from pre-commit.com hooks

b8eb9b5

for more information, see https://pre-commit.ci

Merge branch 'master' into lammpsdata_improper

a28094d

Use get_boundingbox instead of boundingbox.

b1f96bc

bc118 approved these changes Jul 1, 2021

View reviewed changes

justinGilmer approved these changes Jul 8, 2021

View reviewed changes

justinGilmer merged commit dbc1cdf into mosdef-hub:master Jul 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for AMBER proper and improper dihedrals in write_lammpsdata #896

Support for AMBER proper and improper dihedrals in write_lammpsdata #896

rwsmith7531 commented May 4, 2021 •

edited

Loading

codecov bot commented May 5, 2021 •

edited

Loading

rmatsum836 left a comment

rmatsum836 commented May 6, 2021 •

edited

Loading

rmatsum836 commented May 6, 2021

rmatsum836 commented May 6, 2021

rwsmith7531 commented May 7, 2021

rwsmith7531 commented May 7, 2021

rsdefever commented May 7, 2021 •

edited

Loading

bc118 commented May 10, 2021 •

edited

Loading

rsdefever commented May 13, 2021

bc118 left a comment

bc118 Jun 28, 2021

justinGilmer Jun 30, 2021

rwsmith7531 Jul 1, 2021

rwsmith7531 Jul 1, 2021

justinGilmer left a comment

justinGilmer Jun 30, 2021

justinGilmer Jun 30, 2021

rwsmith7531 Jul 1, 2021

bc118 Jul 1, 2021

rwsmith7531 Jul 1, 2021

bc118 Jul 1, 2021

rwsmith7531 Jul 1, 2021

bc118 Jul 1, 2021

bc118 left a comment

justinGilmer left a comment

Support for AMBER proper and improper dihedrals in write_lammpsdata #896

Support for AMBER proper and improper dihedrals in write_lammpsdata #896

Conversation

rwsmith7531 commented May 4, 2021 • edited Loading

PR Summary:

PR Checklist

codecov bot commented May 5, 2021 • edited Loading

Codecov Report

rmatsum836 left a comment

Choose a reason for hiding this comment

rmatsum836 commented May 6, 2021 • edited Loading

rmatsum836 commented May 6, 2021

rmatsum836 commented May 6, 2021

rwsmith7531 commented May 7, 2021

rwsmith7531 commented May 7, 2021

rsdefever commented May 7, 2021 • edited Loading

bc118 commented May 10, 2021 • edited Loading

rsdefever commented May 13, 2021

bc118 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinGilmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bc118 left a comment

Choose a reason for hiding this comment

justinGilmer left a comment

Choose a reason for hiding this comment

rwsmith7531 commented May 4, 2021 •

edited

Loading

codecov bot commented May 5, 2021 •

edited

Loading

rmatsum836 commented May 6, 2021 •

edited

Loading

rsdefever commented May 7, 2021 •

edited

Loading

bc118 commented May 10, 2021 •

edited

Loading