-
Notifications
You must be signed in to change notification settings - Fork 100
Description
Is your feature request related to a problem? Please describe.
LJPME is implemented in some capacity in the toolkit, but it not part of the SMIRNOFF spec. This adds a decent amount of complexity to handling and testing non-bonded methods/OpenMM exports.
- Allowed as a
vdWHandler.methodvalue:openff-toolkit/openff/toolkit/typing/engines/smirnoff/parameters.py
Lines 3575 to 3577 in 1ba11eb
method = ParameterAttribute( default="cutoff", converter=_allow_only(["cutoff", "PME"]) ) - Somewhat dealt with when wrangling OpenMM forces:
openff-toolkit/openff/toolkit/typing/engines/smirnoff/parameters.py
Lines 3638 to 3649 in 1ba11eb
# If we're using PME, then the only possible openMM Nonbonded type is LJPME if self.method == "PME": # If we're given a nonperiodic box, we always set NoCutoff. Later we'll add support for CutoffNonPeriodic if topology.box_vectors is None: force.setNonbondedMethod(openmm.NonbondedForce.NoCutoff) # if (topology.box_vectors is None): # raise SMIRNOFFSpecError("If vdW method is PME, a periodic Topology " # "must be provided") else: force.setNonbondedMethod(openmm.NonbondedForce.LJPME) force.setCutoffDistance(self.cutoff) force.setEwaldErrorTolerance(1.0e-4) - More dealing with
NonbondedForcequirks:openff-toolkit/openff/toolkit/typing/engines/smirnoff/parameters.py
Lines 3876 to 3885 in 1ba11eb
# First, check whether the vdWHandler set the nonbonded method to LJPME, because that means # that electrostatics also has to be PME if (current_nb_method == openmm.NonbondedForce.LJPME) and ( self.method != "PME" ): raise IncompatibleParameterError( "In current Open Force Field Toolkit implementation, if vdW " "treatment is set to LJPME, electrostatics must also be PME " "(electrostatics treatment currently set to {}".format(self.method) ) - Added test complexity: https://github.com/openforcefield/openff-toolkit/blob/master/openff/toolkit/tests/test_forcefield.py#L363-L460
It's covered in CodeCov's opinion, but I don't think any substantive tests cover LJPME.
Describe the solution you'd like
It seems straightforward to suggest that either the implementation should be removed or it should be added to the SMIRNOFF spec.
Describe alternatives you've considered
We could leave it be, but that's likely to produce maintenance issues, bad behavior and/or user experience; implementation should follow specification, not vice versa. Deprecating and then removing LJPME functionality should be straightforward, or expanding the SMIRNOFF spec to allow method="pme" in the vdW section (openforcefield/standards#11) would sync things up.
Additional context
As far as I know, nobody is using LJPME from the toolkit; if they were, it might take a significant amount of modifications (either tinkering with the OpenMM System or using a force field that does not follow SMIRNOFF).
I don't think re-fitting with LJPME is on the next year or so of the science roadmap, but there might be some interest in using it long-term. I'm pretty sure it's implemented in all major simulation engines.
I'm probably missing some history, but I couldn't quickly find anything documented on this.