gmso icon indicating copy to clipboard operation
gmso copied to clipboard

Add LayeredDihedral and LayeredImproper support

Open umesh-timalsina opened this issue 3 years ago • 5 comments

Closes #561.

Checklist

  • [X] Class Definitions
  • [x] Topology Changes (WIP)
  • [x] Tests
  • [ ] Other changes to formats/external modules (May be suited for a different PR)

CC:

  • @rsdefever for cassandramcf/parmed handling @CalCraven for networkx handling @daico007 for mbuild/openmm changes

@bc118 additional comments (10-24-2023):

Multiple Dihedral types (equations) not accepted (Example: CHARMM style with Periodic and Harmonic dihedrals)

Multiple dihedral types should be accepted for both the proper and improper dihedrals. This is the standard input in the CHARMM format. However, in CHARMM, the harmonic dihedrals (proper and improper) are listed as periodic with n=0 (yes, this can be confusing.

We should allow different dihedral types, entering correctly the periodic and harmonic forms as the equations define (see the attached XML). In MoSDeF-GOMC, we will read the proper and improper harmonic dihedrals and write them as the proper and improper periodic dihedrals with n=0 for the CHARMM FF style (required for CHARMM style format - yes, it is confusing). Each writer should handle this separately for their suited engines, where this specific formatted example is for NAMD and the future GOMC once harmonic dihedrals are added.

This is critical if we ever want to be able to use the CHARMM FF in MoSDeF force fields/simulations.

I have provided an example of a proper periodic and harmonic dihedral that fails due to the different dihedral styles.

gmso_ff_with_periodic_and_harmonic_dihedrals.zip

umesh-timalsina avatar Jul 13 '21 19:07 umesh-timalsina

Codecov Report

Merging #569 (ca1afa1) into main (3a387c7) will decrease coverage by 0.06%. The diff coverage is 90.97%.

@@            Coverage Diff             @@
##             main     #569      +/-   ##
==========================================
- Coverage   90.79%   90.72%   -0.07%     
==========================================
  Files          56       56              
  Lines        4626     4699      +73     
==========================================
+ Hits         4200     4263      +63     
- Misses        426      436      +10     
Impacted Files Coverage Δ
gmso/core/improper.py 90.90% <87.50%> (-9.10%) :arrow_down:
gmso/core/topology.py 95.89% <89.13%> (-0.58%) :arrow_down:
gmso/core/dihedral.py 96.55% <95.34%> (-3.45%) :arrow_down:
gmso/__init__.py 100.00% <100.00%> (ø)
gmso/abc/abstract_connection.py 97.87% <100.00%> (ø)
gmso/abc/gmso_base.py 97.36% <100.00%> (+0.03%) :arrow_up:

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 3a387c7...ca1afa1. Read the comment docs.

codecov[bot] avatar Jul 13 '21 20:07 codecov[bot]

Perhaps something to watch out for:

Traceback (most recent call last):
  File "/Users/ryandefever/Desktop/test_serialize/bmim/run.py", line 9, in <module>
    top = from_parmed(pmd)
  File "/Users/ryandefever/repos/mosdef/gmso/gmso/external/convert_parmed.py", line 232, in from_parmed
    top.update_topology()
  File "/Users/ryandefever/repos/mosdef/gmso/gmso/core/topology.py", line 947, in update_topology
    self.update_connection_types()
  File "/Users/ryandefever/repos/mosdef/gmso/gmso/core/topology.py", line 614, in update_connection_types
    if isinstance(c.connection_type, AngleType):
AttributeError: 'LayeredDihedral' object has no attribute 'connection_type'

rsdefever avatar Jul 16 '21 20:07 rsdefever

Perhaps something to watch out for:

Traceback (most recent call last):
  File "/Users/ryandefever/Desktop/test_serialize/bmim/run.py", line 9, in <module>
    top = from_parmed(pmd)
  File "/Users/ryandefever/repos/mosdef/gmso/gmso/external/convert_parmed.py", line 232, in from_parmed
    top.update_topology()
  File "/Users/ryandefever/repos/mosdef/gmso/gmso/core/topology.py", line 947, in update_topology
    self.update_connection_types()
  File "/Users/ryandefever/repos/mosdef/gmso/gmso/core/topology.py", line 614, in update_connection_types
    if isinstance(c.connection_type, AngleType):
AttributeError: 'LayeredDihedral' object has no attribute 'connection_type'

@rsdefever Thanks for the catch. cfd9125 adds the check while updating connection types.

umesh-timalsina avatar Jul 16 '21 20:07 umesh-timalsina

This pull request introduces 2 alerts when merging fa751fa3b3417ccabd54c5025f075048efbc6c1b into 00f329cfbe1decc087172ddc9421ed804c58e44b - view on LGTM.com

new alerts:

  • 2 for Unused import

lgtm-com[bot] avatar Jul 27 '21 15:07 lgtm-com[bot]

Can we add a test where there are dihedrals of different functional forms in the layered dihedral? The actual handling of that will be writer-dependent, but we can support that as we develop the writers.

justinGilmer avatar Jul 27 '21 17:07 justinGilmer