Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add LayeredDihedral and LayeredImproper support #569

Open
wants to merge 41 commits into
base: old_main
Choose a base branch
from
Open

Add LayeredDihedral and LayeredImproper support #569

wants to merge 41 commits into from

Conversation

umesh-timalsina
Copy link
Member

@umesh-timalsina umesh-timalsina commented Jul 13, 2021 •
edited by bc118

Closes #561.

Checklist

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

CC:

@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

@codecov
Copy link

codecov bot commented Jul 13, 2021 •
edited

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%) ⬇️
gmso/core/topology.py 95.89% <89.13%> (-0.58%) ⬇️
gmso/core/dihedral.py 96.55% <95.34%> (-3.45%) ⬇️
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%) ⬆️

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.

@rsdefever
Copy link
Member

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'

@umesh-timalsina
Copy link
Member Author

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 umesh-timalsina marked this pull request as ready for review July 20, 2021 15:15
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 27, 2021

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

new alerts:

  • 2 for Unused import

@justinGilmer
Copy link
Collaborator

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
justinGilmer requested changes Aug 4, 2021
View reviewed changes
Copy link
Collaborator

@justinGilmer justinGilmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Co-reviewed by @umesh-timalsina @daico007

Aside from some changes with adding a dihedral with the same hash to an indexed set, this should be g2g.

gmso/core/dihedral.py Outdated Show resolved Hide resolved
gmso/core/dihedral.py Outdated Show resolved Hide resolved
gmso/core/improper.py Outdated Show resolved Hide resolved
gmso/core/improper.py Outdated
super().__setattr__(key, value)

@validator("improper_types_", pre=True, always=True)
def validate_dihedral_types(cls, improper_types):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def validate_dihedral_types(cls, improper_types):
def validate_improper_types(cls, improper_types):

gmso/core/topology.py Outdated Show resolved Hide resolved
gmso/core/topology.py Outdated Show resolved Hide resolved
@justinGilmer justinGilmer self-requested a review October 7, 2021 17:40
justinGilmer
justinGilmer requested changes Feb 16, 2022
View reviewed changes
Copy link
Collaborator

@justinGilmer justinGilmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small docstring change and then this is g2g for me.

gmso/core/dihedral.py Outdated Show resolved Hide resolved
@CalCraven CalCraven mentioned this pull request Oct 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinGilmer justinGilmer justinGilmer requested changes

@rsdefever rsdefever Awaiting requested review from rsdefever

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Parmed -> GMSO loses layered dihedrals
4 participants
@umesh-timalsina @rsdefever @justinGilmer @daico007