Add ChargeIncrementModelHandler #471

j-wags · 2019-12-05T04:33:21Z

jchodera · 2019-12-05T19:03:11Z

The-SMIRNOFF-force-field-format.md

 </ChargeIncrementModel>
 ```
 The sum of formal charges for the molecule or fragment will be used to determine the total charge the molecule or fragment will possess.

 `<ChargeIncrementModel>` provides several optional attributes to control its behavior:
 * The `number_of_conformers` attribute (default: `"10"`) is used to specify how many conformers will be generated for the molecule (or capped fragment) prior to charging.
 * The `quantum_chemical_method` attribute (default: `"AM1"`) is used to specify the quantum chemical method applied to the molecule or capped fragment.
-* The `partial_charge_method` attribute (default: `"CM2"`) is used to specify how uncorrected partial charges are to be generated from the quantum chemical wavefunction. Later additions will add restrained electrostatic potential fitting (RESP) capabilities.   
+* The `partial_charge_method` attribute (default: `"Mulliken"`) is used to specify how uncorrected partial charges are to be generated from the quantum chemical wavefunction. Later additions will add restrained electrostatic potential fitting (RESP) capabilities.   


In our last discussion, I think we wanted to combine quantum_chemical_method and partial_charge_method to instead have a general two-stage procedure:

Stage 1: Produce initial charges via some QM or ML method: AM1-Mulliken, graph convolutional network, VCharge, etc.

Stage 2: Correct the initial charges with BCCs

That will require more significant revision of the proposed ChargeIncrementModel spec.

Ok, I thought we had been trending toward this decision, but I couldn't find it in writing anywhere. I'm going to have partial_charge_method be the winner here (so, for example, partial_charge_method="AM1-Mulliken"). For the ToolkitRegistry, each toolkit will have a supports_charge_method(method) function which takes a method string and returns a bool indicating whether it can calculate charges using that method.

The partial charge method may itself have other parameters, so it might be useful to split this into two separate tags for the different stages.

When we had discussed this, I think we discussed generalizing it into perhaps two tags: an initial charging tag (that might take optional parameters) and a BondChargeIncrement tag (that describes how to modify charges).

Happy to chat further if you want to discuss ideas for making this flexible and simple to implement.

lgtm-com · 2019-12-07T01:46:05Z

This pull request introduces 1 alert when merging 643a750 into e3a8856 - view on LGTM.com

new alerts:

1 for Unused local variable

…dices changed

The-SMIRNOFF-force-field-format.md

lgtm-com · 2020-06-09T01:09:10Z

This pull request introduces 1 alert and fixes 1 when merging 3d3c9ef into 3de67ca - view on LGTM.com

new alerts:

1 for Module is imported more than once

fixed alerts:

1 for Unused import

dotsdl · 2020-06-09T21:50:14Z

Reviewing!

dotsdl

Great work on this @j-wags! It's a very well-put-together implementation, with a lot of well-thought-out details.

Most of my inline comments are minor. Some additional notes below:

Should we add an explicit test for ToolkitWrapper._check_n_conformers? In fact, that method could be made into a staticmethod, since it really doesn't depend on self at all. A test of this should check that all the logic flow control works as expected.
When are we planning to deprecate ToolkitAM1BCC?
What do we expect to happen if the ChargeIncrementModel section in a SMIRNOFF spec has number_of_conformers=0? Does our machinery currently handle this case, as we'd expect for the gasteiger method?
Remove the warning in the SMIRNOFF spec that ChargeIncrementModel is not yet implemented.

openforcefield/tests/test_forcefield.py

openforcefield/typing/engines/smirnoff/parameters.py

dotsdl · 2020-06-12T01:27:17Z

openforcefield/utils/toolkits.py

+        if partial_charge_method == "am1bccnosymspt":
+            optimize = False
+            symmetrize = False
+            quacpac_status = oequacpac.OEAssignCharges(oemol, charge_method['oe_charge_method'](optimize, symmetrize))


Can you add a small comment on why this partial charge method requires additional parameters?

Added

# The OpenFF toolkit has always supported a version of AM1BCC with no geometry optimization # or symmetry correction. So we include this keyword to provide a special configuration of quacpac # if requested.

dotsdl · 2020-06-16T00:17:25Z

openforcefield/topology/molecule.py

-                    result = toolkit_registry.call('from_object', other, allow_undefined_stereo=allow_undefined_stereo)
-                except NotImplementedError:
-                    pass
+                    result = toolkit_registry.call('from_object',


What is the intention behind this block? It's not clear to me.

As in, what is the behavior we want to support here?

Oh, I thought about this so much, I didn't write down what the successful logic means. D'oh!

Added

# Each ToolkitWrapper may provide a from_object method, which turns some particular type(s) # of object into OFFMols. For example, RDKitToolkitWrapper's from_object method will # return an OFFMol if provided with an RDMol, or raise a ValueError if it is provided # an OEMol (or anything else). This makes the assumption that any non-ValueError errors raised # by the toolkit _really are_ bad and should be raised immediately, which may be a bad assumption.

openforcefield/topology/molecule.py

dotsdl · 2020-06-16T02:26:35Z

openforcefield/tests/test_forcefield.py

+        top = Topology.from_molecules([create_ethanol(), create_reversed_ethanol()])
+        sys = ff.create_openmm_system(top)
+        nonbonded_force = [force for force in sys.getForces() if isinstance(force, openmm.NonbondedForce)][0]
+        expected_charges = [0.2, -0.15, -0.05, 0., 0., 0., 0., 0., 0.,


I'd like to walk through this one with you, just to make sure I'm thinking about it correctly @j-wags.

dotsdl · 2020-06-16T16:36:52Z

openforcefield/tests/test_toolkits.py

+        assert -1.e-5 < charge_sum.value_in_unit(unit.elementary_charge) < 1.e-5
+
+    @pytest.mark.skipif(not OpenEyeToolkitWrapper.is_available(), reason='OpenEye Toolkit not available')
+    @pytest.mark.parametrize("partial_charge_method", ['am1bcc', 'am1-mulliken'])


Is there a reason we aren't including gasteiger here?

Yes -- Gasteiger isn't conformer-dependent, so this test (ensuring that charge methods generate different charges for different conformations) doesn't apply to it. Added
Skip Gasteiger because it isn't conformer dependent.
to the test docstring

dotsdl · 2020-06-16T16:56:56Z

openforcefield/tests/test_toolkits.py

+
+    @pytest.mark.skipif(not RDKitToolkitWrapper.is_available() or not AmberToolsToolkitWrapper.is_available(),
+                        reason='RDKitToolkit and AmberToolsToolkit not available')
+    @pytest.mark.parametrize("partial_charge_method", ['am1bcc', 'am1-mulliken'])


Same question as above: why no gasteiger?

Is it because gasteiger doesn't require conformers at all?

Yup. Added the same text to the docstring as above.

lgtm-com · 2020-06-17T02:02:33Z

This pull request introduces 1 alert and fixes 1 when merging 982fa49 into d0318bf - view on LGTM.com

new alerts:

1 for Module is imported more than once

fixed alerts:

1 for Unused import

Co-authored-by: David Dotson <[email protected]>

lgtm-com · 2020-06-17T23:31:28Z

This pull request introduces 1 alert and fixes 1 when merging d7e092a into d0318bf - view on LGTM.com

new alerts:

1 for Module is imported more than once

fixed alerts:

1 for Unused import

…rgeincrementhandler

lgtm-com · 2020-06-18T00:52:47Z

This pull request introduces 2 alerts and fixes 1 when merging fff6051 into d0318bf - view on LGTM.com

new alerts:

1 for Module is imported more than once
1 for Unused local variable

fixed alerts:

1 for Unused import

j-wags · 2020-06-18T00:57:44Z

Worked on the inline comments today. I'll pick back up on your "overall" comments tomorrow!

j-wags · 2020-06-19T01:01:37Z

Should we add an explicit test for ToolkitWrapper._check_n_conformers? In fact, that method could be made into a staticmethod, since it really doesn't depend on self at all. A test of this should check that all the logic flow control works as expected.

Regarding the tests, good idea. I've added them

Regarding converting to staticmethod, I don't think it's a good long-term idea. We're going to convert ToolkitWrappers to have a cache of previously-performed operations, which would forbid us from using staticmethods and classmethods (since the only place a static or classmethod could store a cache is on the class itself, which is undesirable). For that reason, we're moving toward making all ToolkitWrapper methods into instance methods.

When are we planning to deprecate ToolkitAM1BCC?

Since it's in the already-published Parsley and smirnoff99Frosst FFs, never.

What do we expect to happen if the ChargeIncrementModel section in a SMIRNOFF spec has number_of_conformers=0? Does our machinery currently handle this case, as we'd expect for the gasteiger method?

I've updated the formal_charges example XML to have 0 confs defined (which it should have always been. D'oh!) to ensure that we can correctly handle cases where a FF defined n_conformers=0

Remove the warning in the SMIRNOFF spec that ChargeIncrementModel is not yet implemented.

Good catch. Removed.

This should address everything you pointed out. I'll be leading the 0.7.0 release, so let's work through #572 first and I'll resolve any conflicts with this once it's merged.

lgtm-com · 2020-06-19T01:08:19Z

This pull request introduces 1 alert and fixes 1 when merging e363a70 into d0318bf - view on LGTM.com

new alerts:

1 for Module is imported more than once

fixed alerts:

1 for Unused import

lgtm-com · 2020-06-19T21:19:21Z

This pull request introduces 1 alert and fixes 1 when merging e910488 into a2f1ff3 - view on LGTM.com

new alerts:

1 for Module is imported more than once

fixed alerts:

1 for Unused import

lgtm-com · 2020-06-19T22:22:34Z

This pull request introduces 1 alert and fixes 1 when merging efe98bb into 9553e9c - view on LGTM.com

new alerts:

1 for Module is imported more than once

fixed alerts:

1 for Unused import

j-wags added 5 commits December 4, 2019 15:20

Fix typo in chargeincrementhandler example offxml

28cb5a6

Add basic tests for chargeincrementmodelhandler

0c58167

Fix smirnoff docs CIM example

c0308f0

Update basic CIMH tests

5f7dae8

Add method compatibility matrix to ATTKW

28680ba

jchodera reviewed Dec 5, 2019

View reviewed changes

j-wags added 2 commits December 6, 2019 17:37

Implemented partial charge method resolution test

3a26190

standardize partial charge calc apis

643a750

j-wags added 15 commits December 6, 2019 18:36

standardize partial charge calc "raises"es

1e369b4

fix variable name

3ff5783

Resolve #348

71a097f

remove qc_method from CIM API, default n_confs=1

7268ec1

Resolve #465

4d865f7

Update tests for #348

c9d63b2

Correct error types in charge calc, add tests for new charge methods

96629df

Add builtintoolkitwrapper and tests

c44d990

pass partial_charge_method through in molecule.calc_part_charges

6153fdd

update release history

880faec

update CIM spec docs to default n_confs 1 and remove quantum method

ddd69e8

Fix CIM spec example

84fd4ea

Enforce use of dict to prevent chargeincrement SMIRKS from getting in…

2611145

…dices changed

Finish initial implementation of chargeincrementmodelhandler

0711222

add create_reversed_ethanol and CIM test

e3e47fb

j-wags mentioned this pull request Dec 17, 2019

Partial, formal, and total charges are reported with inconsistent units #465

Closed

jchodera reviewed Dec 18, 2019

View reviewed changes

The-SMIRNOFF-force-field-format.md Show resolved Hide resolved

j-wags added 4 commits December 18, 2019 20:26

fix tests for changes in #465. Atom.formal_charge accepts int or Quant

8826d06

clarify reason for overriding transformed_dict_cls in CIMH

3f6fbb8

Fix tests for new API/defaults to CIMH

f3840fb

Update error handling in molecule.__init__ for more specific exceptions

6a22604

j-wags added 3 commits June 8, 2020 17:48

simplify release notes

0a465fc

fix docstring example

e89687a

Merge branch 'master' into chargeincrementhandler

3d3c9ef

j-wags requested a review from dotsdl June 9, 2020 01:01

mattwthompson mentioned this pull request Jun 12, 2020

Decouple partial charge assignment from creation of OpenMM systems #619

Closed

dotsdl approved these changes Jun 16, 2020

View reviewed changes

dotsdl and others added 2 commits June 16, 2020 10:26

Small cosmetic fixes

92c0df4

Merge branch 'master' into chargeincrementhandler

982fa49

j-wags changed the title ~~[WIP] Add ChargeIncrementModelHandler~~ Add ChargeIncrementModelHandler Jun 17, 2020

Update openforcefield/typing/engines/smirnoff/parameters.py

d7e092a

Co-authored-by: David Dotson <[email protected]>

j-wags added 2 commits June 17, 2020 17:43

add fixes per Dotson review

49d4dc3

Merge remote-tracking branch 'origin/chargeincrementhandler' into cha…

fff6051

…rgeincrementhandler

add more fixes per Dotson review

e363a70

Merge branch 'master' into chargeincrementhandler

e910488

Merge branch 'master' into chargeincrementhandler

efe98bb

j-wags merged commit b3f5050 into master Jun 19, 2020

j-wags deleted the chargeincrementhandler branch June 19, 2020 23:48

SimonBoothroyd mentioned this pull request Jun 23, 2020

OpenFF Toolkit 0.7.0 Compatability SimonBoothroyd/nonbonded#31

Merged

1 task

j-wags mentioned this pull request Feb 15, 2021

Use ELF10 for system creation when OpenEyeToolkitWrapper is available #837

Closed

j-wags mentioned this pull request May 26, 2023

Passing keyword arguments to underlying toolkits #1610

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ChargeIncrementModelHandler #471

Add ChargeIncrementModelHandler #471

j-wags commented Dec 5, 2019 •

edited

Loading

jchodera Dec 5, 2019

j-wags Dec 5, 2019 •

edited

Loading

jchodera Dec 6, 2019

lgtm-com bot commented Dec 7, 2019

lgtm-com bot commented Jun 9, 2020

dotsdl commented Jun 9, 2020

dotsdl left a comment

dotsdl Jun 12, 2020

j-wags Jun 18, 2020

dotsdl Jun 16, 2020

dotsdl Jun 16, 2020

j-wags Jun 18, 2020

dotsdl Jun 16, 2020

dotsdl Jun 16, 2020

j-wags Jun 18, 2020

dotsdl Jun 16, 2020

dotsdl Jun 16, 2020

j-wags Jun 18, 2020

lgtm-com bot commented Jun 17, 2020

lgtm-com bot commented Jun 17, 2020

lgtm-com bot commented Jun 18, 2020

j-wags commented Jun 18, 2020

j-wags commented Jun 19, 2020

lgtm-com bot commented Jun 19, 2020

lgtm-com bot commented Jun 19, 2020

lgtm-com bot commented Jun 19, 2020

Add ChargeIncrementModelHandler #471

Add ChargeIncrementModelHandler #471

Conversation

j-wags commented Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

j-wags Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Dec 7, 2019

lgtm-com bot commented Jun 9, 2020

dotsdl commented Jun 9, 2020

dotsdl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 17, 2020

lgtm-com bot commented Jun 17, 2020

lgtm-com bot commented Jun 18, 2020

j-wags commented Jun 18, 2020

j-wags commented Jun 19, 2020

lgtm-com bot commented Jun 19, 2020

lgtm-com bot commented Jun 19, 2020

lgtm-com bot commented Jun 19, 2020

j-wags commented Dec 5, 2019 •

edited

Loading

j-wags Dec 5, 2019 •

edited

Loading