Skip to content

Commit e363a70

Browse files
j-wagsj-wags
committed
add more fixes per Dotson review
1 parent fff6051 commit e363a70

File tree

5 files changed

+91
-7
lines changed

5 files changed

+91
-7
lines changed

‎The-SMIRNOFF-force-field-format.md‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -225,8 +225,6 @@ For example, to ensure water molecules are assigned partial charges for [TIP3P](
225225

226226
### `<ChargeIncrementModel>`: Small molecule and fragment charges
227227

228-
.. warning:: This functionality is not yet implemented and will appear in a future version of the toolkit. This area of the SMIRNOFF spec is under further consideration. Please see `Issue 208 on the Open Force Field Toolkit issue tracker <https://github.com/openforcefield/openforcefield/issues/208>`_.
229-
230228
In keeping with the AMBER force field philosophy, especially as implemented in small molecule force fields such as [GAFF](http://ambermd.org/antechamber/gaff.html), [GAFF2](https://mulan.swmed.edu/group/gaff.php), and [parm@Frosst](http://www.ccl.net/cca/data/parm_at_Frosst/), partial charges for small molecules are usually assigned using a quantum chemical method (usually a semiempirical method such as [AM1](https://en.wikipedia.org/wiki/Austin_Model_1)) and a [partial charge determination scheme](https://en.wikipedia.org/wiki/Partial_charge) (such as [CM2](http://doi.org/10.1021/jp972682r) or [RESP](http://doi.org/10.1021/ja00074a030)), then subsequently corrected via charge increment rules, as in the highly successful [AM1-BCC](https://dx.doi.org/10.1002/jcc.10128) approach.
231229

232230
Here is an example:

‎openforcefield/tests/test_forcefield.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@
240240

241241
xml_charge_increment_model_formal_charges = '''
242242
<SMIRNOFF version="0.3" aromaticity_model="OEAroModel_MDL">
243-
<ChargeIncrementModel version="0.3" number_of_conformers="1" partial_charge_method="formal_charge"/>
243+
<ChargeIncrementModel version="0.3" number_of_conformers="0" partial_charge_method="formal_charge"/>
244244
</SMIRNOFF>
245245
'''
246246

@@ -1520,7 +1520,7 @@ def test_charge_increment_model_partially_overlapping_matches_both_apply(self):
15201520
test_charge_increment_model_ff = '''
15211521
<SMIRNOFF version="0.3" aromaticity_model="OEAroModel_MDL">
15221522
<Electrostatics version="0.3" method="PME" scale12="0.0" scale13="0.0" scale14="0.833333" cutoff="9.0 * angstrom"/>
1523-
<ChargeIncrementModel version="0.3" number_of_conformers="1" partial_charge_method="formal_charge">
1523+
<ChargeIncrementModel version="0.3" number_of_conformers="0" partial_charge_method="formal_charge">
15241524
<ChargeIncrement smirks="[#6X4:1]([#1:2])([#1:3])([#1:4])" charge_increment1="0.3*elementary_charge" charge_increment2="-0.1*elementary_charge" charge_increment3="-0.1*elementary_charge" charge_increment4="-0.1*elementary_charge"/>
15251525
<ChargeIncrement smirks="[#6X4:1][#6X4:2][#8]" charge_increment1="0.05*elementary_charge" charge_increment2="-0.05*elementary_charge"/>
15261526
</ChargeIncrementModel>

‎openforcefield/tests/test_toolkits.py‎

Lines changed: 87 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222

2323
from openforcefield.utils.toolkits import (OpenEyeToolkitWrapper, RDKitToolkitWrapper,
2424
AmberToolsToolkitWrapper, BuiltInToolkitWrapper, ToolkitRegistry,
25+
ToolkitWrapper,
2526
GAFFAtomTypeWarning, UndefinedStereochemistryError,
2627
ChargeMethodUnavailableError, IncorrectNumConformersError,
2728
IncorrectNumConformersWarning)
@@ -2166,8 +2167,93 @@ def test_assign_partial_charges_wrong_n_confs(self):
21662167
use_conformers=molecule.conformers,
21672168
strict_n_conformers=True)
21682169

2170+
class TestToolkitWrapper:
2171+
"""Test the ToolkitWrapper class"""
2172+
def test_check_n_conformers(self):
2173+
"""Ensure that _check_n_conformers is working properly"""
2174+
tkw = ToolkitWrapper()
2175+
mol = create_ethanol()
2176+
2177+
## Test molecule with no conformers
2178+
# Check with no min or max should pass
2179+
tkw._check_n_conformers(mol, 'nocharge')
2180+
# Check with min=1 should warn
2181+
with pytest.warns(IncorrectNumConformersWarning,
2182+
match="has 0 conformers, but charge method 'nocharge' expects at least 1"):
2183+
tkw._check_n_conformers(mol, 'nocharge', min_confs=1)
2184+
# Check with min=1 and strict_n_conformers should raise an error
2185+
with pytest.raises(IncorrectNumConformersError,
2186+
match="has 0 conformers, but charge method 'nocharge' expects at least 1"):
2187+
tkw._check_n_conformers(mol, 'nocharge', min_confs=1, strict_n_conformers=True)
2188+
# Check with min=1, max=1 and strict_n_conformers should raise an error
2189+
with pytest.raises(IncorrectNumConformersError,
2190+
match="has 0 conformers, but charge method 'nocharge' expects exactly 1"):
2191+
tkw._check_n_conformers(mol, 'nocharge', min_confs=1, max_confs=1, strict_n_conformers=True)
2192+
# Check with min=1, max=2 and strict_n_conformers should raise an error
2193+
with pytest.raises(IncorrectNumConformersError,
2194+
match="has 0 conformers, but charge method 'nocharge' expects between 1 and 2"):
2195+
tkw._check_n_conformers(mol, 'nocharge', min_confs=1, max_confs=2, strict_n_conformers=True)
2196+
# Check with max=1 should pass
2197+
tkw._check_n_conformers(mol, 'nocharge', max_confs=1, strict_n_conformers=True)
2198+
2199+
## Test molecule with conformers
2200+
# Add some conformers
2201+
mol.generate_conformers(n_conformers=1)
2202+
for _ in range(9):
2203+
mol.add_conformer(mol.conformers[0])
2204+
2205+
# Check with no min or max should pass
2206+
tkw._check_n_conformers(mol, 'nocharge')
2207+
2208+
## min_confs checks
2209+
# Check with min=1 should be fine
2210+
tkw._check_n_conformers(mol, 'nocharge', min_confs=1)
2211+
# Check with min=10 should be fine
2212+
tkw._check_n_conformers(mol, 'nocharge', min_confs=10)
2213+
# Check with min=11 should warn
2214+
with pytest.warns(IncorrectNumConformersWarning,
2215+
match="has 10 conformers, but charge method 'nocharge' expects at least 11"):
2216+
tkw._check_n_conformers(mol, 'nocharge', min_confs=11)
2217+
# Check with min=11 and strict_n_conformers should raise an error
2218+
with pytest.raises(IncorrectNumConformersError,
2219+
match="has 10 conformers, but charge method 'nocharge' expects at least 11"):
2220+
tkw._check_n_conformers(mol, 'nocharge', min_confs=11, strict_n_conformers=True)
2221+
2222+
## max_confs checks
2223+
# Check with max=1 and strict_n_conformers should raise an error
2224+
with pytest.raises(IncorrectNumConformersError,
2225+
match="has 10 conformers, but charge method 'nocharge' expects at most 1"):
2226+
tkw._check_n_conformers(mol, 'nocharge', max_confs=1, strict_n_conformers=True)
2227+
# Check with max=10 and strict_n_conformers should be OK
2228+
tkw._check_n_conformers(mol, 'nocharge', max_confs=10, strict_n_conformers=True)
2229+
# Check with max=11 and strict_n_conformers should be OK
2230+
tkw._check_n_conformers(mol, 'nocharge', max_confs=11, strict_n_conformers=True)
2231+
2232+
## min_confs and max_confs checks
2233+
# Check with max=10 and min=10 and strict_n_conformers should be OK
2234+
tkw._check_n_conformers(mol, 'nocharge', min_confs=10, max_confs=10, strict_n_conformers=True)
2235+
# Check with max=10 and min=9 and strict_n_conformers should be OK
2236+
tkw._check_n_conformers(mol, 'nocharge', min_confs=9, max_confs=10, strict_n_conformers=True)
2237+
# Check with max=11 and min=10 and strict_n_conformers should be OK
2238+
tkw._check_n_conformers(mol, 'nocharge', min_confs=10, max_confs=11, strict_n_conformers=True)
2239+
# Check with max=11 and min=9 and strict_n_conformers should be OK
2240+
tkw._check_n_conformers(mol, 'nocharge', min_confs=9, max_confs=11, strict_n_conformers=True)
2241+
# Check with min=9 and max=9 and strict_n_conformers should raise an error
2242+
with pytest.raises(IncorrectNumConformersError,
2243+
match="has 10 conformers, but charge method 'nocharge' expects exactly 9"):
2244+
tkw._check_n_conformers(mol, 'nocharge', min_confs=9, max_confs=9, strict_n_conformers=True)
2245+
# Check with min=1 and max=9 and strict_n_conformers should raise an error
2246+
with pytest.raises(IncorrectNumConformersError,
2247+
match="has 10 conformers, but charge method 'nocharge' expects between 1 and 9"):
2248+
tkw._check_n_conformers(mol, 'nocharge', min_confs=1, max_confs=9, strict_n_conformers=True)
2249+
# Check with min=11 and max=12 and strict_n_conformers should raise an error
2250+
with pytest.raises(IncorrectNumConformersError,
2251+
match="has 10 conformers, but charge method 'nocharge' expects between 11 and 12"):
2252+
tkw._check_n_conformers(mol, 'nocharge', min_confs=11, max_confs=12, strict_n_conformers=True)
2253+
2254+
21692255
class TestToolkitRegistry:
2170-
"""Test the ToolkitRegistry"""
2256+
"""Test the ToolkitRegistry class"""
21712257

21722258
@pytest.mark.skipif(not OpenEyeToolkitWrapper.is_available(), reason='OpenEye Toolkit not available')
21732259
def test_register_openeye(self):

‎openforcefield/typing/engines/smirnoff/forcefield.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1249,7 +1249,7 @@ def _get_parameter_handler_class(self, tagname):
12491249
tagname)
12501250
msg += "Known parameter handler class tags are {}".format(self._parameter_handler_classes.keys())
12511251
raise KeyError(msg)
1252-
return ph_classte
1252+
return ph_class
12531253

12541254
def __getitem__(self, val):
12551255
"""

‎openforcefield/utils/toolkits.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,7 @@ def _check_n_conformers(self,
309309
If the wrong number of conformers is attached to the input molecule, and strict_n_conformers is True.
310310
"""
311311
import warnings
312-
n_confs = len(molecule.conformers)
312+
n_confs = molecule.n_conformers
313313
wrong_confs_msg = f"Molecule '{molecule}' has {n_confs} conformers, " \
314314
f"but charge method '{partial_charge_method}' expects"
315315
exception_suffix = "You can disable this error by setting `strict_n_conformers=False' " \

0 commit comments

Comments
 (0)