Skip to content
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

vdW switch width not used #882

Closed
SimonBoothroyd opened this issue Mar 25, 2021 · 6 comments
Closed

vdW switch width not used #882

SimonBoothroyd opened this issue Mar 25, 2021 · 6 comments

Comments

@SimonBoothroyd
Copy link
Contributor

Describe the bug

This vdWHandler exposes a switch_width property but as far as I can tell this doesn't get used anywhere in the code.

To Reproduce

from openff.toolkit.topology import Molecule
from openff.toolkit.typing.engines.smirnoff import ForceField
from simtk import openmm, unit

molecule = Molecule.from_smiles("CCC")

# Create a system from a default FF.
force_field = ForceField("openff-1.3.0.offxml")

system_original = force_field.create_openmm_system(molecule.to_topology())
system_original_str = openmm.XmlSerializer.serialize(system_original)

# Create a system from a modified version of the FF
force_field = ForceField("openff-1.3.0.offxml")
force_field.get_parameter_handler("vdW").switch_width = 1000.0 * unit.angstroms

system_modified = force_field.create_openmm_system(molecule.to_topology())
system_modified_str = openmm.XmlSerializer.serialize(system_modified)

assert system_original_str != system_modified_str

Output
The full error message (may be large, that's fine. Better to paste too much than too little.)

Computing environment (please complete the following information):

  • Operating system - OSX
  • Output of running conda list
openff-forcefields        1.3.0              pyh44b312d_0    conda-forge
openff-toolkit-base       0.9.1              pyhd8ed1ab_1    conda-forge
openmm                    7.5.0           py38hbe4764b_6_apple    conda-forge

Additional context
Add any other context about the problem here.

@mattwthompson
Copy link
Member

mattwthompson commented Mar 25, 2021

Is the spec being followed at the moment?

# Copy README example

In [5]: nbf = openmm_system.getForces()[0]

In [6]: nbf.getUseSwitchingFunction()
Out[6]: False

In [7]: nbf.getSwitchingDistance()
Out[7]: Quantity(value=-1.0, unit=nanometer)

i.e. is a switching function even being applied?

@mattwthompson mattwthompson added this to the 0.9.2 milestone Mar 30, 2021
@SimonBoothroyd SimonBoothroyd mentioned this issue Mar 31, 2021
5 tasks
@mattwthompson
Copy link
Member

Also see linked issue for some confusion about what switch_width in the SMIRNOFF spec actually defines

@davidlmobley
Copy link
Contributor

I don't have specific feedback on details of this because I haven't worked with the relevant part of OpenMM enough. Generally, though, I'd expect there to be a VDW switch region over which VDW interactions are switched smoothly to zero (switching function) and the switch width ought to define this. If it's not being done we should fix it, I think.

@davidlmobley
Copy link
Contributor

Did this get cleared up by addressing OFF-EP 000 so now we know what to implement?

If we're going to have switch-width in the API it has to do something, otherwise we need to remove it, I think.

I also just commented on #896 in case that removes the blocker.

@mattwthompson
Copy link
Member

I don't think would be fixed by an OFF-EP since the problem is the implementation not following the spec. The linked PR should close this, though, provided it's accepted and we've determined the behavior change is acceptable.

@mattwthompson
Copy link
Member

This has been resolved, and actually a new error will be raised when trying to use a nonsense switch width:

In [3]: from openff.toolkit.topology import Molecule
   ...: from openff.toolkit.typing.engines.smirnoff import ForceField
   ...: from openff.units import unit
   ...:
   ...: molecule = Molecule.from_smiles("CCC")
   ...:
   ...: # Create a system from a default FF.
   ...: force_field = ForceField("openff-1.3.0.offxml")
   ...:
   ...: system_original = force_field.create_openmm_system(molecule.to_topology())
   ...: system_original_str = openmm.XmlSerializer.serialize(system_original)
   ...:
   ...: # Create a system from a modified version of the FF
   ...: force_field = ForceField("openff-1.3.0.offxml")
   ...: force_field.get_parameter_handler("vdW").switch_width = 1000.0 * unit.angstroms
   ...:
   ...: system_modified = force_field.create_openmm_system(molecule.to_topology())
   ...: system_modified_str = openmm.XmlSerializer.serialize(system_modified)
   ...:
   ...: assert system_original_str != system_modified_str
/Users/mattthompson/mambaforge/envs/test/lib/python3.10/site-packages/openff/interchange/components/interchange.py:339: UserWarning: Automatically up-converting BondHandler from version 0.3 to 0.4. Consider manually upgrading this BondHandler (or <Bonds> section in an OFFXML file) to 0.4 or newer. For more details, see https://openforcefield.github.io/standards/standards/smirnoff/#bonds.
  warnings.warn(
---------------------------------------------------------------------------
UnsupportedCutoffMethodError              Traceback (**I'm cutting out some lines of this**)

File ~/mambaforge/envs/test/lib/python3.10/site-packages/openff/interchange/interop/openmm/_nonbonded.py:763, in _apply_switching_function(vdw_handler, force)
    758 switching_distance = to_openmm_quantity(
    759     vdw_handler.cutoff - vdw_handler.switch_width,
    760 )
    762 if switching_distance._value < 0:
--> 763     raise UnsupportedCutoffMethodError(
    764         "Found a `switch_width` greater than the cutoff distance. It's not clear "
    765         "what this means and it's probably invalid. Found "
    766         f"switch_width{vdw_handler.switch_width} and cutoff {vdw_handler.cutoff}",
    767     )
    769 force.setUseSwitchingFunction(True)
    770 force.setSwitchingDistance(switching_distance)

UnsupportedCutoffMethodError: Found a `switch_width` greater than the cutoff distance. It's not clear what this means and it's probably invalid. Found switch_width1000.0 angstrom and cutoff 9.0 angstrom

Slightly modifying this to use a sensible but non-default switch width triggers a difference in the produced openmm.System:

In [4]: from openff.toolkit.topology import Molecule
   ...: from openff.toolkit.typing.engines.smirnoff import ForceField
   ...: from openff.units import unit
   ...:
   ...: molecule = Molecule.from_smiles("CCC")
   ...:
   ...: # Create a system from a default FF.
   ...: force_field = ForceField("openff-1.3.0.offxml")
   ...:
   ...: system_original = force_field.create_openmm_system(molecule.to_topology())
   ...: system_original_str = openmm.XmlSerializer.serialize(system_original)
   ...:
   ...: # Create a system from a modified version of the FF
   ...: force_field = ForceField("openff-1.3.0.offxml")
   ...: force_field.get_parameter_handler("vdW").switch_width = 0.4444 * unit.angstroms
   ...:
   ...: system_modified = force_field.create_openmm_system(molecule.to_topology())
   ...: system_modified_str = openmm.XmlSerializer.serialize(system_modified)
   ...:
   ...: assert system_original_str != system_modified_str
/Users/mattthompson/mambaforge/envs/test/lib/python3.10/site-packages/openff/interchange/components/interchange.py:339: UserWarning: Automatically up-converting BondHandler from version 0.3 to 0.4. Consider manually upgrading this BondHandler (or <Bonds> section in an OFFXML file) to 0.4 or newer. For more details, see https://openforcefield.github.io/standards/standards/smirnoff/#bonds.
  warnings.warn(

# No errors

For good measure, the control case fails as expected - so the differences must be due to modifying the switch width in the force field!

In [5]: from openff.toolkit.topology import Molecule
   ...: from openff.toolkit.typing.engines.smirnoff import ForceField
   ...: from openff.units import unit
   ...:
   ...: molecule = Molecule.from_smiles("CCC")
   ...:
   ...: # Create a system from a default FF.
   ...: force_field = ForceField("openff-1.3.0.offxml")
   ...:
   ...: system_original = force_field.create_openmm_system(molecule.to_topology())
   ...: system_original_str = openmm.XmlSerializer.serialize(system_original)
   ...:
   ...: # Create a system from a modified version of the FF
   ...: force_field = ForceField("openff-1.3.0.offxml")
   ...: # force_field.get_parameter_handler("vdW").switch_width = 0.4444 * unit.angstroms
   ...:
   ...: system_modified = force_field.create_openmm_system(molecule.to_topology())
   ...: system_modified_str = openmm.XmlSerializer.serialize(system_modified)
   ...:
   ...: assert system_original_str != system_modified_str
/Users/mattthompson/mambaforge/envs/test/lib/python3.10/site-packages/openff/interchange/components/interchange.py:339: UserWarning: Automatically up-converting BondHandler from version 0.3 to 0.4. Consider manually upgrading this BondHandler (or <Bonds> section in an OFFXML file) to 0.4 or newer. For more details, see https://openforcefield.github.io/standards/standards/smirnoff/#bonds.
  warnings.warn(
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
Cell In[5], line 20
     17 system_modified = force_field.create_openmm_system(molecule.to_topology())
     18 system_modified_str = openmm.XmlSerializer.serialize(system_modified)
---> 20 assert system_original_str != system_modified_str

AssertionError:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants