Changes to SMIRNOFF spec documentation

[j-wags]

This issue is to keep a record of spec changes/questions to make as I find them.

• GBSA model: radius_units should be radius_unit to match up with other _unit fields in the spec (and because the _unit suffix on an attrib is used to trigger special behavior during parsing)
• It's not clear whether SA model defaults to None or ACE

[j-wags]

• VDW tags' switch=8.0*angstrom should be changed to switch='8.0' switch_unit='angstrom'
• Same with VDW tags' cutoff

[j-wags]

• change switch-width attribute of Electrostatics tag to switch_width (underscore) in compliance with other multi-word attributes
• separate out cutoff_unit and switch_width_unit attributes in Electrostatics tag

[j-wags]

Add a note clarifying that it you should not use the same attrib name for a section header attrib and a parameter attrib (for example, I shouldn't have a section header "length" and "length_unit", and also a ParameterType "length" field, because the units might get crossed).

The spec does not currently do this. However, the parser for unit-bearing fields will have undefined behavior if someone makes a new ParameterHandler that uses the same name for a unit-bearing attrib in both the header and each parameter, and then somehow sets them to different unit systems.

[j-wags]

• Resolve whether torsions get fractional bond orders

[j-wags]

• The vdW tag goes up to a 1-5 scaling term. Electrostatics only goes up to 1-4. Should Electrostatics have a 1-5?

[jchodera]

The vdW tag goes up to a 1-5 scaling term. Electrostatics only goes up to 1-4. Should Electrostatics have a 1-5?

Sure, let's make them both go to 1-5.

[j-wags]

Always put increment-able numbers at the end of attr names. For example charge1increment should become chargeincrement1

[j-wags]

@jchodera @davidlmobley Should ImproperTorsions have the same idivf convention as ProperTorsions? I'm mostly asking with regard to the default default_idivf='auto' setting.

My understanding is that this is where our "impropers-get-divided-by-a-factor-of-three" convention stems from, but I could imagine someone defining weird impropers so I don't want to leave "3" hard coded.

Spec reference here: https://open-forcefield-toolkit.readthedocs.io/en/topology/smirnoff.html#propertorsions

[j-wags]

Update Changelog

[jchodera]

@jchodera @davidlmobley Should ImproperTorsions have the same idivf convention as ProperTorsions? I'm mostly asking with regard to the default default_idivf='auto' setting.

My understanding is that this is where our "impropers-get-divided-by-a-factor-of-three" convention stems from, but I could imagine someone defining weird impropers so I don't want to leave "3" hard coded.

I think we should leave the division by a factor of three hard-coded for our trefoil improper torsions. It's a different circumstance than the idivf for converting AMBER torsions, since we will always compute all three torsions in our trefoil definition.

[j-wags]

@jchodera, the Electrostatics and vdW tags in the SMIRNOFF spec both have cutoff and switch parameters. However, I only see one way to set the switch and cutoff values in an openmm.NonbondedForce. Is the OpenMM convention to use the same cutoff and switch width for both vdW and coulomb forces?

If so, I will allow the switch and cutoff values to be defined in both tags, but will raise an Exception if 1) Two different values are specified 2) No value is specified, but a handler calls for a method that requires a cutoff to be specified

[jchodera]

Is the OpenMM convention to use the same cutoff and switch width for both vdW and coulomb forces?

OpenMM's NonbondedForce requires that the same cutoff be used for all nonbonded forces, but in reality, separate cutoffs can be implemented through the use of special CustomNonbondedForce forces that force the energy to zero at different cutoffs.

The real consideration here should be what information we logically want to capture, since we're just using OpenMM's System as a convenient container class for now. We really want to capture anything that would affect the computed potential energies, but leave optimizations up to the specific molecular mechanics package and hardware implementation.

The documentation in the spec is accurate, in that we need to capture different kinds of information for different cases:

Attributes in the tag specify the scaling terms applied to the energies of 1-2 (scale12, default: 0), 1-3 (scale13, default: 0), and 1-4 (scale14, default: 0.5) interactions, (scale15, default: 1.0), as well as the distance at which a switching function is applied (switch, default: "8.0"), the cutoff (cutoff, default: "9.0"), and long-range dispersion treatment scheme (long_range_dispersion, default: "isotropic"). If Lennard-Jones PME is to be used, switch and cutoff are omitted and long_range_dispersion="PME" is used.

Note that if LJPME is used, switch and cutoff are omitted; if an isotropic dispersion correction is used, we need the switch and cutoff specified.

Electrostatics is a different story. If PME is used, we don't need a cutoff since---provided the PME tolerance is specified---the cutoff just determines how much work is pushed into reciprocal vs direct space since the PME error tolerance will be satisfied either way (though we may later choose to change how we specify that error tolerance). If reaction-field or direct Coulomb is used, we need a cutoff to be specified, and switch-width is optional.

For now, it would make sense to raise an Exception if someone tries to use a different cutoff for electrostatics and Lennard-Jones interactions since we're storing them in the same NonbondedForce. Later, we can relax this limitation as we move toward a more flexible object model.

[j-wags]

Thanks for the quick response!

The real consideration here should be what information we logically want to capture, since we're just using OpenMM's System as a convenient container class for now.

Ah, that's some good context to keep in mind. Ok. I'll go forward with the implementation suggested, and raise exceptions in cases where our current data model can't handle a given configuration.

[jchodera]

Sounds good.

The long-term solution involves developing a common object data model for representing molecular systems that is the same as the data model used to transform between simulation packages. This may be a confluence between OpenMM 8, InterMol, and some footwork to establish common standards among different packages.

[j-wags]

Note: Current supported nonbonded schemes (and the SMIRNOFF values that enable them) are recorded here: https://docs.google.com/presentation/d/1U1ZnB-kaAA1s-lFjTQSI1hHx5kntbOw-H4HlGdV4KCo/edit#slide=id.g4cf85191c0_0_0

[davidlmobley]

@j-wags just confirming that (a) we want to retain the "automatically divide by three" for impropers, and (b) we want to maintain idivf support for torsions. Impropers are a totally separate animal.

[j-wags]

Initial tests to ensure that nonbonded method resolution is being handled correctly have been implemented. The desired behavior is defined here and the actual tests are run here

[jchodera]

@j-wags : Can this be closed now that we have updated all of the documentation?

[mattwthompson]

I skimmed this and I think everything's been implemented.

If there are issues or ambiguities around the text of the SMIRNOFF specification, I encourage users to open issues at https://github.com/openforcefield/standards, where our specifications are currently housed.