-
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
Update smirnoff99Frosst to remove generics in DrugBank #232
Conversation
@bannanc - I'm going to suggest edits to your summary above since this is our record for posterity. Here are a couple of things which are unclear to me:
What's "specified above" mean? I'm guessing you mean "above this in the force field file"? Also, the reference of "the carbon bond" could be more clear -- THIS carbon bond? Or the one above? (Again, presumably you mean the former, but it's easier if it's just clear when you write it the first time so one doesn't have to stop and think about it.) |
… updatesmirff pull changes in master
Merging Master
I fixed some of the comments
@davidlmobley Note - there are a few corner cases we don't quite catch in the full DrugBank set. My next plan is to close all the issues this pull request addresses and then open a single (LONG) issue with chemistry that we think needs to be explored further. That will include anything we got from GAFF2, things we don't cover now (i.e. boron/silicon), and anything where we made things fairly generic to cover our bases. This will serve as a source of molecules/functional groups for the undergrad in our group during the summer. |
@bannanc :
This plan sounds excellent. Reviewing FF changes in a cursory way (since I already reviewed all the individual issues in detail) now. |
And, once merged, you will also migrate these changes to github.com/open-forcefield-group/openforcefield and github.com/open-forcefield-group/smirnoff99frosst, @bannanc ? (We're working towards removing smirnoff from smarty). |
# Updated: Many times during Fall 2016 - Winter 2017, most recently Fri. March 3, 2017 | ||
# Date: only last line in this section is used, all others in list will be ignored. | ||
DATE | ||
# Updated: Many times during Fall 2016 - Winter 2017, small change with eithers Fri. March 3, 2017 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"ethers" not "eithers". :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @bannanc !
@davidlmobley if you're ok with what I did with the aromatic nitrogen torsion then I think this is good to know with the knowledge that we'll probably have another one in the not too distant future. |
@bannanc - tests are failing because the tests load
|
(It's OK with me if you go ahead and merge once tests pass, @bannanc !) |
I didn't change anything in the XML format because I didn't want to break it, but presumably we'll need to update that too
@bannanc - looks like py27 and py35 passed on your last commit, but py34 is failing. Checking. |
The most recent run failed on python3 and not python2, but the failure said it didn't get 100% on the AlkEthOH answers... I don't think we changed anything in the AlkEthOH parameters on this set. I'll see if the same problem repeats on this test |
Well, that's really weird... Py34 test failed with something you haven't touched in here:
I think the SMIRKY test may have accepted a very rare move which resulted in losing some coverage of the AlkEthOH set, perhaps, just in this one case? Looks like it should be good to merge when you are ready, @bannanc . |
Nevermind, it already has 2 steps and temperature = 0.0 it shouldn't ever accept moves that decrease the score... I don't know what caused the failed test assuming this one passes I'll merge it. |
This pull request will include parameters needed to match most of the chemistry in the larger set DrugBank. It still is no metals and molecules with <= 200 heavy atoms.
In this message I will list all of the changes I've made with links to issues where they have been discussed.
Parm99 or Parm@Frosst
Bonds
[#16X3+0,#16X4]
) (issue Find missing parameters in smirff99Frosst for DrugBank #216 )[#16X2,#16X1-1,#16X3+1:1]
sulfurs. We do not want the hypervalent sulfur bond to over ride these S-C bonds. So the SMIRKS pattern for hypervalent sulfur singly bound to carbon was set as[#16X4,#16X3!+1:1]-[#6]
X1
requirement for nitrogen in nitrile (issue Find missing parameters in smirff99Frosst for DrugBank #216 )#7X1
and#7X2&(*#*)
so it is the triple bond that is important.X4
requirement for the most generic S-C bond. (issue Find missing parameters in smirff99Frosst for DrugBank #216 )X2
requirement from sulfur in S-H bond (smirnoff99Frosst issue#31)[#6X4:1]-[#8X2:2]
to[#6:1]-[#8:2]
to cover corner casesAngles
#7X2
angle for linear case (R-C#N+1-R)Torsions
[#16X3+0,#16X4]
) (smirnoff99Frosst issue#40 )#16X3
, but in smirnoff there are some higher priority cases with#16X3+1
we wanted to remove the neutral requirement without overwriting those.Estimates from GAFF2
Bonds
Angles
Torsions