Modified RB and Periodic torsions #615
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
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 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? |
|
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. |
|
It is the form of an improper but it says dihedral/torsion, which are very different. So at a minimum, this should be renamed. |
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 |
bc118
changed the title
| </Parameters> | ||
| </BondType> | ||
| </BondTypes> | ||
| <AngleTypes expression="0.5 * k * (theta-theta_eq)**2"> | ||
| <ParametersUnitDef parameter="theta_eq" unit="radian"/> | ||
| <ParametersUnitDef parameter="k" unit="kJ/(mol*radian**2)"/> | ||
| <AngleType name="AngleType1" type1='C' type2='C' type3="C"> | ||
| <AngleType name="AngleType1" type1="'C" type2="'C'" type3="C"> |
There was a problem hiding this comment.
| <AngleType name="AngleType1" type1="'C" type2="'C'" type3="C"> | |
| <AngleType name="AngleType1" type1="C" type2="C" type3="C"> |
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "name": "PeriodicTorsionPotential", | |||
| "expression": "k * (1 + cos(n * phi - phi_eq))", | |||
| "expression": "k0 + k1 * (1 + cos(1 * phi - phi_eq1)) + k2 * (1 + cos(2 * phi - phi_eq2)) + k3 * (1 + cos(3 * phi - phi_eq3)) + k4 * (1 + cos(4 * phi - phi_eq4)) + k5 * (1 + cos(5 * phi - phi_eq5))", | |||
There was a problem hiding this comment.
After spending some more thought on this, we might want to be careful with this eqn, especially regarding how many terms we want to support. As in, can we keep the old untouched and create a different/derivative for 5 terms, 4 terms, etc. More ideas/thoughts are welcome
There was a problem hiding this comment.
Yeah, I hear what you mean. I think, If you do not supported fixed terms it may be harder to parse when you don’t know what is there for the engines. I believe We should be consistent, meaning if RB and OPLS is fixed this should be too.
There was a problem hiding this comment.
I also don’t believe I saw anything ever past n=5 or power=5, which are both there converting to one or the other. The fixed term way in my mind is consistent for programming and the readers. I will make the general solution working in GOMC so it is the same as others.
I’m for the fixed one to the 5th power since all other ones RB and OPLS are fixed too. , but open if there is a specific reason not to do it that way.
Interested to hear what other think.
Though, many derivatives means that the FFs won’t be directly shareable. Whatever we pick we should stick to it as a general solution and not make derivative of it, or it will be hard or impossible to share FFs without manually rewriting or fixing to some extent.
There was a problem hiding this comment.
After additional though, if we decide to use the original periodic form, we should have additional check for the input variables. For example:
-
The valuables must me k1, … kx, n1, …, nx or phi1, …, phix. This will ensure the FFs are shareable.
-
k1 and phi1 must have n1=1 . Kx and phix must have nx =x
There was a problem hiding this comment.
Is this PR ready to close then, based on this comment thread?
This will fix the failed test in test_lammps
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.