Port #896 to biopolymer-topology-refactor branch

[mattwthompson]

This PR ports #896 (by @SimonBoothroyd) to the "refactor" branch according to my proposal here, thereby closing it.

Two modifications include

1. Changes after clarifying that switch_width is the difference between the "switching distance" (i.e. setSwitchingDistance) and cutoff distance. For example, typical values are 9 Angstrom cutoff, 1 Angstrom switch with, and therefore an 8 Angstrom switching distance.
2. Raising an exception (InvalidSwitchingDistanceError) if the switch width is greater than the switch distance, which is not a sensible pairing of parameters.

• Tag issue being addressed
• Add tests
• Update docstrings/documentation, if applicable
• Lint codebase
• Update changelog

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (topology-biopolymer-refactor@61b35a8). Click here to learn what that means.
The diff coverage is n/a.

[j-wags]

I approve making new reference energies using the code in this branch to get the tests passing. Edit: I'm a fool, that's not used in the failing tests

[mattwthompson]

Oh wow, I didn't see that GBSA tests are also failing ... those might take some more thinking. Presumably the switching function isn't applied in the Amber files, but I haven't looked into it nor am I as familiar with that code as others.

[j-wags]

Ohhh darn, I was too hasty. The reference energies thing won't fix these tests. We'll need to update these lines to feed in new cutoff/switching kwargs OpenMM's AmberPrmtopFile.createSystem call

[mattwthompson]

Well, I tried to thread that through, but ran up against it not being supported and/or valid (I don't yet have the familiarity with GBSA to say which):

>>> from openmm import GBSAOBCForce
>>> x = GBSAOBCForce()
>>> x.getUseSwitchingFunction()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'GBSAOBCForce' object has no attribute 'getUseSwitchingFunction'
>>> dir(x)
['CutoffNonPeriodic', 'CutoffPeriodic', 'NoCutoff', '__class__', '__copy__', '__deepcopy__', '__delattr__', '__dict__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattribute__', '__getstate__', '__gt__', '__hash__', '__init__', '__init_subclass__', '__le__', '__lt__', '__module__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__setstate__', '__sizeof__', '__str__', '__subclasshook__', '__swig_destroy__', '__weakref__', 'addParticle', 'getCutoffDistance', 'getForceGroup', 'getName', 'getNonbondedMethod', 'getNumParticles', 'getParticleParameters', 'getSoluteDielectric', 'getSolventDielectric', 'getSurfaceAreaEnergy', 'setCutoffDistance', 'setForceGroup', 'setName', 'setNonbondedMethod', 'setParticleParameters', 'setSoluteDielectric', 'setSolventDielectric', 'setSurfaceAreaEnergy', 'this', 'thisown', 'updateParametersInContext', 'usesPeriodicBoundaryConditions']

More verbose demo here: https://gist.github.com/mattwthompson/77df8268933462d6dcfa100d3bf9f2aa

[mattwthompson]

Might be an actual bug in OpenMM (openmm/openmm#3488). Whether or not it is, pinning to a yet-to-be-released version of OpenMM is not tenable, so I might have to add some logic to work around this.

[mattwthompson]

I decided against including every possible safeguards in either the switch_width validator or the lines that actually set it on the force. If somebody wants to muck around with a negative switch width, a switch width greater than the cutoff, etc., there should be some alarm bells going off at other steps such that it's easy to diagnose. With updated wording in the SMIRNOFF spec coming soon, sane defaults written in the source code of the parameter handler (9 A cutoff, 1 A switch width), and the high likelihood that users will simply copy+paste whatever is in our mainline force fields (also 9 A, 1 A) I think this ranks low on potential points of failure to be concerned about.

Barring surprise results in CI, these changes to not require updating the (minimal) reference energies in this repo. It's not too surprising, since they only cover systems of singe small molecules. So the behavior is only tested via the unit tests.

To get the GBSA tests online, I decided to go with an imperfect but low-friction solution. I simply turned off the switching functions in those tests and only those tests. The failures were due to OpenMM's AmberPrmtopFile not being able to create a system with a switching function on its non-bonded force and an implicit solvent model [1]. Note that this only happens with AmberPrmtopFile.createSystem and does not seem to be a functional limitation of the systems created with ForceField.create_openmm_system. Other solutions are more involved (switching to ParmEd inside of this non-API function) or induce headache timebombs (hardcoding different behavior based on the version of OpenMM installed [2].

Now, the only failing tests is our good friend test_chemical_environment_matches_OE.

---

1. I've submitted a bugfix to OpenMM to fix this, but this PR does not depend on that.
2. Eventually I want to move many of these tests into Interchange / deleted from here - they constitute a large portion of some of the larger files in this codebase, many are skipped, and they take longer than most of the other unit tests.

[j-wags]

Thanks for the great PR, @mattwthompson. We talked a lot about whether we should raise an error for trying to set a switch_width for LJPME, but ultimately I don't think it's worth changing the behavior to raise an error or do anything else. With regard to what Interchange should do in that case, let's discuss that in the context of the "interchange as export machinery" project plan and approval.

CI-wise, feel free to ignore the failing OpenEye test - That one's on me. However the failing OpenMM tests on Mac ARE blocking to merging this.