-
Notifications
You must be signed in to change notification settings - Fork 8
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.
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
Cross-check SMIRFF energies vs parm@frosst on full AlkEthOH set (finished, tests added) #60
Conversation
Resolves #58 |
I still have an issue with
It seems the expected SMARTS is not being matched in this case:
Troubleshooting. Ah, yes, SMARTS typo in XML and prototype file. We can't have |
Are you sure the |
@jchodera : Both |
Ha! Excellent point. |
Now 253 out of ~900 in the "chains" subset pass, so we're making real headway. Next up,
(now with units, thanks to debugging I did earlier on the system checker in openmoltools - choderalab/openmoltools#228 ). OK, but this is water, so our tests should bypass this as the AMBER number is drawing on TIP3P water and our AlkEthOH parameter file is not attempting to reproduce that. Skipping in tests. |
All 900+ chains now pass. Onward to rings. First up,
Well, that's interesting. This is a fun compound, SMILES It's perhaps worth noting that C1-C2-C3-O1 (0, 1, 2, 3) and C3-C2-C1-O1 (2, 1, 0, 3) ought to be symmetry-equivalent torsions I think, but that is NOT what's going on here. The (0, 3, 2, 1) torsion is C1-O1-C3-C2, or CT-OS-CT-CT. Weird. Torsional matches here:
|
Seems like the relevant term should be <Proper smirks="[#6X4:1]-[#6X4:2]-[#8X2:3]-[#6X4:4]" idivf1="1" periodicity1="3" phase1="0.0" k1="0.383" idivf2="1" periodicity2="2" phase2="180.0" k2="0.1"/> <!-- CT-CT-OS-CT from frcmod.Frosst_AlkEthOH --> the CT-CT-OS-CT torsion, which is not consistent with either ordering here. This DOES get one match here, so possibly this is just a problem with handling of torsions in cyclic molecules in the system_checker... ?? Relevant code is here: https://github.com/choderalab/openmoltools/blob/master/openmoltools/system_checker.py#L388 |
So if I manually look at this SMIRKS ( Maybe AMBER and/or OpenMM is doing something internally to reorder torsions for rings. I'll have to revisit tomorrow/soon, as I'm at a loss for now. |
I suppose the above atoms could be covered by the generic |
I think you're right---I expect the problem is with rendering the Perhaps you could instead use the same |
I'll check that out, @jchodera . Thanks! |
Hmm, @jchodera - it's not actually looking to me like that's the issue. I've output the entire table of torsions identified for this molecule and the AMBER-originated system really is picking up an 0-3-2-1 (C1-O1-C3-C2 / CT-OS-CT-CT) torsion which is not being picked up when generating from SMIRFF - i.e. for SMIRFF we don't get an 0-3-2-1 torsion NOR an 1-2-3-0 torsion. (units degrees and kilocalories_per_mole):
I'm seeing O1 appearing in 19 torsions in the AMBER case and 16 torsions in OpenMM. Here's the full output (AMBER at the top). I'm walking through it to see if I can tell what's wrong.
|
OK, well this is interesting. Focusing in on all torsions involving O1 and parsing down to only what is different (i.e. removing all torsions which are the same) I get these for O1:
Top is AMBER prmtop, bottom is SMIRFF. Top corresponds to CT-CT-CT-OS, CT-OS-CT-CT (two torsions), and OS-CT-CT-OH (two torsions). This coudl be a bug relating to sets -- specifically, Christopher suggests that ONLY for four membered rings can you have non-equivalent torsions which correspond to the same set. Specifically, here, the AMBER (apparently correct) CT-CT-CT-OS torsion (or OS-CT-CT-CT) is atoms [ 0, 1, 2, 3 ] AND ALSO [ 2, 1, 0, 3] or |
OK, I just tested on cyclobutane and it fails as well. |
Yup, specific four-membered ring bug, @jchodera , as C1CCC1 fails but C1CCCC1 and C1CCCCC1 pass. Will troubleshoot code looking for problematic set comparisons. |
Not seeing any apparently problematic set comparisons. The only thing I see which I wonder about is the use of the "unique" flag when finding SMIRKS matches, but I can't find the docs on that. Investigating. |
If I switch https://github.com/open-forcefield-group/smarty/blob/master/smarty/forcefield.py#L244 to
I'm going to blast through the rest of the molecules again with this set to False to see if it creates any other problems. |
…d rings. Breaks no other cases.
Now everything passes. Next stop: implement some energy comparison checks as part of the standard tests, then this can be closed and SMIRFF v1 will be done! |
Added new Travis/nose tests to cross-check OpenMM energies beginning from SMIRFF parm@frosst against AMBER .prmtop and .crd parm@frosst. The entire AlkEthOH set now passes locally on my machine, and several molecules are now automatically tested as part of the tests. |
Going ahead and merging because it passed before I updated the README.md. Travis is stuck for some reason I think. |
@bannanc - you were curious about non-unique torsion matches. See particularly this thread, and especially #60 (comment) and #60 (comment). Basically if we set |
(To be more specific, @bannanc , imagine a four-membered ring involving carbons 1, 2, 3, and 4. There is a torsion 1-2-3-4 and a torsion 4-3-2-1 which of course are the same torsion. But there are also other torsions involving the same four atoms (2-3-4-1, 3-4-1-2) which are not the same torsion. Per the OpenEye docs, since these all involve the same four atoms, only one of them is unique.) |
Continuing my energy validation by cross-checking full set.
Found another parm@frosst/parm99 set of bugs where
H2-CT-CT-O*
andH3-CT-CT-O*
were erroneously inheriting a generic torsion (X-CT-CT-X
). Checking continues.