Add Smarts Strings for Chlorobenzene and Benzonitrile #308

Argon1999 · 2020-01-23T17:35:04Z

PR Summary:

Added Smarts Strings for Chlorobenzene and Benzonitrile, they atom typed correctly. Tested Density and Self Diffusivity got decent results when comparing to literature values.

PR Checklist

Includes appropriate unit test(s)
Appropriate docstring(s) are added/updated
Code is (approximately) PEP8 compliant
Issue(s) raised/addressed?

ahy3nz · 2020-01-25T22:10:35Z

Toluene, propanenitrile, o-xylene, 124-trimethylbenzene, 2-methylphenol, 3-methylphenol, 4-methylphenol, and ethyl benzene failed for testopls::test_atomtyping.

Atomtyping conflicts: 260 vs 145, 261 vs 754, 262 vs 753. Check back at the definitions for those atom types, it's possible your PR's definitions are too generic, or maybe those old atom-types need to be amended

mattwthompson · 2020-01-27T17:14:50Z

Also please add some test molecules in MOL2 format and update the implemented_opls_tests.txt file. You can look through existing files in foyer/opls_validation/ or i.e. #278 for reference

rmatsum836 · 2020-01-27T17:17:56Z

Thanks for your feedback. @Argon1999 is currently running some simulations of bulk chlorobenzene and benzonitrile to verify several properties. Afterwards, we will add some molecules, update implemented_opls_test.txt, and check the atom typing conflicts.

rmatsum836 · 2020-01-28T18:20:01Z

I think tests will pass now. Should just need to add the mol2 files and update implemented_opls_tests.txt now.

codecov · 2020-01-28T18:44:56Z

Codecov Report

Merging #308 into master will decrease coverage by 2.11%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
- Coverage   86.90%   84.78%   -2.12%     
==========================================
  Files          14       14              
  Lines        1252     1275      +23     
==========================================
- Hits         1088     1081       -7     
- Misses        164      194      +30

rmatsum836 · 2020-01-30T16:48:28Z

Update on this: Benzonitrile is atom typing. We replaced the gro and top files in the opls_validation directory with a mol2 file, per #309. However we are now getting an error due to missing dihedral types. This also brings up the question of how we want to handle assert_dihedral_params for testing atom types.

One of the dihedral types we're missing is in the benonitrile ring (CA-CA-CA-CA), but we can't find it in the force field files in GROMACS. Searching other sources to see if we can find the parameters for this dihedral.

mattwthompson · 2020-01-30T16:51:05Z

Normally I'd go to http://virtualchemistry.org/ (the companion to David van der Spoel's big JCTC paper: https://pubs.acs.org/doi/abs/10.1021/ct200731v) since I think many of our files originally came from there, but it seems to be offline ....

…ing on line 2675

justinGilmer

Awesome additions! Thanks @Argon1999 and @rmatsum836 ! Just a comment/suggestion left below and after that, LGTM!

foyer/forcefields/xml/oplsaa.xml

Co-Authored-By: Justin Gilmer <[email protected]>

rmatsum836 · 2020-02-06T16:25:42Z

Thanks for fixing this @Argon1999. @mosdef-hub/mosdef-contributors I think this is good to go if I could get another pair of eyes on this.

rmatsum836 · 2020-02-06T16:28:25Z

JK, test_from_mbuild_customtype is failing for some reason on two of the AZP builds.

daico007 · 2020-02-06T16:29:21Z

This fail seems to relate to the new box class, so I am looking into this now

rmatsum836 · 2020-02-06T18:28:06Z

Tests are now passing. @mosdef-hub/mosdef-contributors this should be good to be reviewed now.

mattwthompson

Am I missing chlorobenzene files? I don't see them in this PR or the opls_validation folder of the master branch

mattwthompson · 2020-02-06T18:28:43Z

foyer/forcefields/xml/oplsaa.xml

+  <Type name="opls_260" class="CA" element="C" mass="12.011" def="[C;X3;r6]([C;X2;%opls_261])" overrides = "opls_145" desc="Benzonitrile C(CN)" doi="10.1021/ja9621760"/>
+  <Type name="opls_261" class="CZ" element="C" mass="12.011" def="[C;X2]([C;X3;r6])" overrides = "opls_754" desc="Benzonitrile: carbon in nitrile group (CN) " doi="10.1021/ja9621760"/>
+  <Type name="opls_262" class="NZ" element="N" mass="14.0067" def="[N;X1][C;%opls_261]" overrides = "opls_753" desc="Benzonitrile Nitrogen(benzene-C)N" doi="10.1021/ja9621760"/>
+  <Type name="opls_263" class="CA" element="C" mass="12.011" def="[C;X3;r6]1(Cl)[C;X3;r6][C;X3;r6][C;X3;r6][C;X3;r6][C;X3;r6]1" overrides="opls_145" desc="Chlorobenzene C(Cl)" doi="10.1021/ja9621760"/>


Suggested change

<Type name="opls_263" class="CA" element="C" mass="12.011" def="[C;X3;r6]1(Cl)[C;X3;r6][C;X3;r6][C;X3;r6][C;X3;r6][C;X3;r6]1" overrides="opls_145" desc="Chlorobenzene C(Cl)" doi="10.1021/ja9621760"/>

<Type name="opls_263" class="CA" element="C" mass="12.011" def="[C;X3;r6](Cl)1[C;X3;r6][C;X3;r6][C;X3;r6][C;X3;r6][C;X3;r6]1" overrides="opls_145" desc="Chlorobenzene C(Cl)" doi="10.1021/ja9621760"/>

I am just curious if this works too, no need to make this change. I wonder if the branch can be defined both inside and outside the ring

@Argon1999 could you add gro and top files for chlorobenzene in a Chlorobenzene directory in opls_validation?

justinGilmer

Lets shorten the amount of cholorbenzenes in the gro file from 200 -> 1, and then we can see if all the tests pass @Argon1999

justinGilmer · 2020-02-11T17:22:34Z

foyer/opls_validation/chlorobenzene/chlorobenzene.gro

@@ -0,0 +1,2403 @@
+GROningen MAchine for Chemical Simulation
+ 2400


Can we update this file to just have 1 cholorobenzene? Will make the testing a bit faster.

rmatsum836 · 2020-02-12T01:20:51Z

Really went down a rabbit hole with this one. I think that we're much better off reading gro and top files compared to reading in a mol2. ParmEd assumes a certain naming convention to grab elements by name, and is unable to read in Chlorine by CL or Cl. However, if a ParmEd structure is initialized with a top file, then the element is read by mass and chlorine is read in correctly.

What caused us problems in this case is that we had both top and mol2 files. In test_atomtyping, we loop through the files in each directory. As a result, the ParmEd structure loaded in by a top file would get overwritten by a structure loaded in by a mol2file. I think we should change the structure of this code where the for loop is broken if we find a top file in the directory.

daico007 · 2020-02-12T01:32:50Z

@rmatsum836 I think use mb.load instead of pmd.load in the unit test for the mol2 case should fix this. We are having the same issues a while ago with this over mbuild, and we decided to enforce molecule's name capitalization before shipping to Parmed to fix that.

rmatsum836 · 2020-02-12T02:01:40Z

@rmatsum836 I think use mb.load instead of pmd.load in the unit test for the mol2 case should fix this. We are having the same issues a while ago with this over mbuild, and we decided to enforce molecule's name capitalization before shipping to Parmed to fix that.

I thought about that too, but wasn't sure if we wanted to go from mb.Compound to pmd.Structure for these tests. However, I don't really see any negatives to this fix.

mattwthompson · 2020-02-12T02:06:23Z

My only (tiny) concern is that we're basing a Foyer test off of a quirk in mBuild. I'm fine preferring MDTraj over ParmEd since it seems like it has the better MOL2 reader. I'll raise an issue there to see if we can clear things up.

rmatsum836 · 2020-02-12T02:07:29Z

Regardless, the tests are passing so should be good to merge. Not sure why there is a drop in coverage though.

ahy3nz · 2020-02-12T02:08:42Z

@rmatsum836 I think use mb.load instead of pmd.load in the unit test for the mol2 case should fix this. We are having the same issues a while ago with this over mbuild, and we decided to enforce molecule's name capitalization before shipping to Parmed to fix that.

I thought about that too, but wasn't sure if we wanted to go from mb.Compound to pmd.Structure for these tests. However, I don't really see any negatives to this fix.

The whole reason we use pmd.load_file is because the resultant pmd.Structure.atoms have both name(element) and type (atom-type). Going back to mbuild loses any atom type information, which is the "answer" used to validate the atom-typing. Similarly, I think mdtraj.load will only retain name information and nothing about atom types that we need to validate

mattwthompson

Tests passing, LGTM

Arjun Bansal added 2 commits January 16, 2020 09:50

Add Smarts String for Chlorobenzene and Benzonitrile

2cda6ab

Change class types of Chlorobenze and benzonitrile types

c046418

Arjun Bansal and others added 2 commits January 28, 2020 11:13

Add recent changes to Benzonitrile Smarts Strings

ed956f5

Fix atomtypes so tests pass

674ec64

Arjun Bansal and others added 4 commits January 30, 2020 11:35

Added missing dihedral coefficient to allow for benzonitrile atom-typ…

030d859

…ing on line 2675

Merge branch 'benzo' of https://github.com/Argon1999/foyer into benzo

efa16b2

Add gro and top files for benzonitrile

3d45742

Merge branch 'benzo' of https://github.com/Argon1999/foyer into benzo

c2ee177

justinGilmer reviewed Feb 3, 2020

View reviewed changes

foyer/forcefields/xml/oplsaa.xml Outdated Show resolved Hide resolved

Argon1999 and others added 2 commits February 6, 2020 09:37

Update foyer/forcefields/xml/oplsaa.xml

ccb4c1c

Co-Authored-By: Justin Gilmer <[email protected]>

Improved descripitions of Benzonitrile and Chlorobenzene smarts strings

1f8e14a

justinGilmer approved these changes Feb 6, 2020

View reviewed changes

mattwthompson requested changes Feb 6, 2020

View reviewed changes

Arjun Bansal and others added 4 commits February 11, 2020 08:32

Add top, gro and mol2 files for chlorobenzene

1e2f452

Fix merge conflict in opls

8618ac8

Change the descriptions to be clearer

f91a72d

Add molecules to tested list

0491581

justinGilmer requested changes Feb 11, 2020

View reviewed changes

Replace chlorobenzene gro and top fles

f3b1fc9

Remove mol2 file and add new gro and top file

d5eb870

rmatsum836 mentioned this pull request Feb 12, 2020

Unify files used in force field validation #309

Open

mattwthompson mentioned this pull request Feb 12, 2020

Element parsing in MOL2 files mosdef-hub/mbuild#690

Closed

mattwthompson approved these changes Feb 13, 2020

View reviewed changes

rmatsum836 approved these changes Feb 13, 2020

View reviewed changes

rmatsum836 merged commit 1e95f06 into mosdef-hub:master Feb 13, 2020

Argon1999 deleted the benzo branch June 17, 2020 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Smarts Strings for Chlorobenzene and Benzonitrile #308

Add Smarts Strings for Chlorobenzene and Benzonitrile #308

Argon1999 commented Jan 23, 2020

ahy3nz commented Jan 25, 2020

mattwthompson commented Jan 27, 2020

rmatsum836 commented Jan 27, 2020

rmatsum836 commented Jan 28, 2020

codecov bot commented Jan 28, 2020 •

edited

Loading

rmatsum836 commented Jan 30, 2020

mattwthompson commented Jan 30, 2020

justinGilmer left a comment

rmatsum836 commented Feb 6, 2020

rmatsum836 commented Feb 6, 2020

daico007 commented Feb 6, 2020

rmatsum836 commented Feb 6, 2020

mattwthompson left a comment

mattwthompson Feb 6, 2020

rmatsum836 Feb 6, 2020

justinGilmer left a comment

justinGilmer Feb 11, 2020

rmatsum836 commented Feb 12, 2020

daico007 commented Feb 12, 2020

rmatsum836 commented Feb 12, 2020

mattwthompson commented Feb 12, 2020

rmatsum836 commented Feb 12, 2020

ahy3nz commented Feb 12, 2020 •

edited

Loading

mattwthompson left a comment

	<Type name="opls_263" class="CA" element="C" mass="12.011" def="[C;X3;r6]1(Cl)[C;X3;r6][C;X3;r6][C;X3;r6][C;X3;r6][C;X3;r6]1" overrides="opls_145" desc="Chlorobenzene C(Cl)" doi="10.1021/ja9621760"/>
	<Type name="opls_263" class="CA" element="C" mass="12.011" def="[C;X3;r6](Cl)1[C;X3;r6][C;X3;r6][C;X3;r6][C;X3;r6][C;X3;r6]1" overrides="opls_145" desc="Chlorobenzene C(Cl)" doi="10.1021/ja9621760"/>

		@@ -0,0 +1,2403 @@
		GROningen MAchine for Chemical Simulation
		2400

Add Smarts Strings for Chlorobenzene and Benzonitrile #308

Add Smarts Strings for Chlorobenzene and Benzonitrile #308

Conversation

Argon1999 commented Jan 23, 2020

PR Summary:

PR Checklist

ahy3nz commented Jan 25, 2020

mattwthompson commented Jan 27, 2020

rmatsum836 commented Jan 27, 2020

rmatsum836 commented Jan 28, 2020

codecov bot commented Jan 28, 2020 • edited Loading

Codecov Report

rmatsum836 commented Jan 30, 2020

mattwthompson commented Jan 30, 2020

justinGilmer left a comment

Choose a reason for hiding this comment

rmatsum836 commented Feb 6, 2020

rmatsum836 commented Feb 6, 2020

daico007 commented Feb 6, 2020

rmatsum836 commented Feb 6, 2020

mattwthompson left a comment

Choose a reason for hiding this comment

mattwthompson Feb 6, 2020

Choose a reason for hiding this comment

rmatsum836 Feb 6, 2020

Choose a reason for hiding this comment

justinGilmer left a comment

Choose a reason for hiding this comment

justinGilmer Feb 11, 2020

Choose a reason for hiding this comment

rmatsum836 commented Feb 12, 2020

daico007 commented Feb 12, 2020

rmatsum836 commented Feb 12, 2020

mattwthompson commented Feb 12, 2020

rmatsum836 commented Feb 12, 2020

ahy3nz commented Feb 12, 2020 • edited Loading

mattwthompson left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 28, 2020 •

edited

Loading

ahy3nz commented Feb 12, 2020 •

edited

Loading