gmso icon indicating copy to clipboard operation
gmso copied to clipboard

Modified RB and Periodic torsions

Open bc118 opened this issue 3 years ago • 6 comments

  • The RB torsion should just use C0 and not C0 * cos(psi)**0. Note this is changing the phi to psi.

  • The periodic torsion should just be similar to RB and go to the 5th power series (i.e., cos(5x) term)

  • We should make a decision between the CHARMM system (all constants spelled out C0-C5..) vs the Periodic term, where they are put in individually. To maximize the usability of the FFs between users, only 1 should be utilized, IMO.

bc118 avatar Dec 15 '21 19:12 bc118

Codecov Report

Merging #615 (37797b0) into master (d592215) will decrease coverage by 0.02%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #615      +/-   ##
==========================================
- Coverage   90.54%   90.51%   -0.03%     
==========================================
  Files          56       56              
  Lines        4525     4525              
==========================================
- Hits         4097     4096       -1     
- Misses        428      429       +1     
Impacted Files Coverage Δ
gmso/external/convert_foyer_xml.py 92.23% <ø> (ø)
gmso/external/convert_parmed.py 90.38% <ø> (ø)
gmso/formats/top.py 92.47% <0.00%> (-1.08%) :arrow_down:

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 d592215...37797b0. Read the comment docs.

codecov[bot] avatar Dec 15 '21 20:12 codecov[bot]

We need to ask if we want to use the Phi variable for RB torsions, as it is used for OPLS and Periodic torsions. RB torsion Phi is not the same as OPLS and Periodic:

RB-torsion-angle = OPLS-torsion-angle - Pi = Phi - PI RB-torsion-angle = Periodic -torsion-angle - Pi = Phi - PI

Maybe we call the RB-torsion-angle Psi like the GROMACS manual (https://manual.gromacs.org/documentation/2019/reference-manual/functions/bonded-interactions.html). I changed it in this PR to Phi for the main equation.

If everyone agrees, I will change the other files accordingly. Thoughts? Comments? OK with this?

bc118 avatar Dec 17 '21 02:12 bc118

Are we naming the HarmonicTorsionPotential.json potential correctly? It seems like we should change the name or equation, or add both and change them accordingly. Thoughts?

It seems LAMMPS calls this a quadratic (https://docs.lammps.org/dihedral_quadratic.html#dihedral-style-quadratic-command)

LAMMPS calls this a harmonic (https://docs.lammps.org/dihedral_harmonic.html)

bc118 avatar Dec 17 '21 02:12 bc118

Are we naming the HarmonicTorsionPotential.json potential correctly? It seems like we should change the name or equation, or add both and change them accordingly. Thoughts?

It seems LAMMPS calls this a quadratic (https://docs.lammps.org/dihedral_quadratic.html#dihedral-style-quadratic-command)

LAMMPS calls this a harmonic (https://docs.lammps.org/dihedral_harmonic.html)

Gromacs cites this equation as a harmonic improper dihedral. So that might be where the current naming convention comes from, but the lammps docs seem to disagree with that convention. { "name": "HarmonicTorsionPotential", "expression": "0.5 * k * (phi - phi_eq)**2", "independent_variables": "phi" }

CalCraven avatar Dec 17 '21 15:12 CalCraven

It is the form of an improper but it says dihedral/torsion, which are very different. So at a minimum, this should be renamed.

bc118 avatar Dec 17 '21 15:12 bc118

We need to ask if we want to use the Phi variable for RB torsions, as it is used for OPLS and Periodic torsions. RB torsion Phi is not the same as OPLS and Periodic:

RB-torsion-angle = OPLS-torsion-angle - Pi = Phi - PI RB-torsion-angle = Periodic -torsion-angle - Pi = Phi - PI

Maybe we call the RB-torsion-angle Psi like the GROMACS manual (https://manual.gromacs.org/documentation/2019/reference-manual/functions/bonded-interactions.html). I changed it in this PR to Phi for the main equation.

If everyone agrees, I will change the other files accordingly. Thoughts? Comments? OK with this?

I agree with this point, being clear and giving these parameters different names hopefully will reduce errors from users missing this distinction. Along the same lines, if the above harmonic torsion potential is changed to an a harmonic improper, the independent variable should be ξ.

I also think we need the proper dihedral form of the function, so I'm in favor of changing the name of this to a quadratic torsion and keeping the current form.

Side note, should we be citing in these .json files some indication of where the functional form was pulled from?

CalCraven avatar Dec 17 '21 15:12 CalCraven