Check for keyword args to createSystem() that are never used (#3242)

* Check for keyword args to createSystem() that are never used

* Fixed incorrect arguments in test cases

wrappers/python/openmm/app/forcefield.py

@@ -46,6 +46,7 @@ import openmm.unit as unit
 from . import element as elem
 from openmm.app.internal.singleton import Singleton
 from openmm.app.internal import compiled
+from openmm.app.internal.argtracker import ArgTracker
 
 # Directories from which to load built in force fields.
 
@@ -1198,6 +1199,7 @@ class ForceField(object):
         args['switchDistance'] = switchDistance
         args['flexibleConstraints'] = flexibleConstraints
         args['drudeMass'] = drudeMass
+        args = ArgTracker(args)
         data = ForceField._SystemData(topology)
         rigidResidue = [False]*topology.getNumResidues()
 
@@ -1368,6 +1370,7 @@ class ForceField(object):
 
         for script in self._scripts:
             exec(script, locals())
+        args.checkArgs(self.createSystem)
         return sys
 
 

wrappers/python/openmm/app/internal/argtracker.py

+from inspect import signature
+
+class ArgTracker(dict):
+    """This class tracks usage of function keyword arguments to detect ones that are never used."""
+
+    def __init__(self, *args):
+        super().__init__(*args)
+        self.accessed = set()
+
+    def __getitem__(self, key):
+        self.accessed.add(key)
+        return super().__getitem__(key)
+
+    def get(self, key, default):
+        self.accessed.add(key)
+        return super().get(key, default)
+
+    def checkArgs(self, fn):
+        """Throw an exception if any argument was never used."""
+        parameters = signature(fn).parameters
+        for key in self:
+            if key not in self.accessed:
+                if key not in parameters or self[key] != parameters[key].default:
+                    raise ValueError(f"The argument '{key}' was specified to {fn.__name__}() but was never used.")

wrappers/python/tests/TestForceField.py

@@ -282,6 +282,18 @@ class TestForceField(unittest.TestCase):
             else:
                 self.assertEqual(trueMass[i], adjustedMass[i])
 
+    def test_UnusedArgs(self):
+        """Test that specifying an argument that is never used throws an exception."""
+        topology = self.pdb1.topology
+        # Using the default value should not raise an exception.
+        self.forcefield1.createSystem(topology, drudeMass=0.4*amu)
+        # Specifying a non-default value should.
+        with self.assertRaises(ValueError):
+            self.forcefield1.createSystem(topology, drudeMass=0.5*amu)
+        # Specifying a nonexistant argument should raise an exception.
+        with self.assertRaises(ValueError):
+            self.forcefield1.createSystem(topology, nonbndedCutoff=1.0*nanometer)
+
     def test_Forces(self):
         """Compute forces and compare them to ones generated with a previous version of OpenMM to ensure they haven't changed."""
 

wrappers/python/tests/TestIntegrators.py

@@ -15,7 +15,7 @@ class TestIntegrators(unittest.TestCase):
         # Create a periodic solvated system with PME
         pdb = PDBFile('systems/alanine-dipeptide-explicit.pdb')
         ff = ForceField('amber99sbildn.xml', 'tip3p.xml')
-        system = ff.createSystem(pdb.topology, cutoffMethod=PME)
+        system = ff.createSystem(pdb.topology, nonbondedMethod=PME)
 
         # Split forces into groups
         for force in system.getForces():
@@ -102,7 +102,7 @@ class TestIntegrators(unittest.TestCase):
         # Create a periodic solvated system with PME
         pdb = PDBFile('systems/alanine-dipeptide-explicit.pdb')
         ff = ForceField('amber99sbildn.xml', 'tip3p.xml')
-        system = ff.createSystem(pdb.topology, cutoffMethod=PME)
+        system = ff.createSystem(pdb.topology, nonbondedMethod=PME)
 
         # Split forces into groups
         for force in system.getForces():
@@ -126,7 +126,7 @@ class TestIntegrators(unittest.TestCase):
         # Create a periodic solvated system with PME
         pdb = PDBFile('systems/alanine-dipeptide-explicit.pdb')
         ff = ForceField('amber99sbildn.xml', 'tip3p.xml')
-        system = ff.createSystem(pdb.topology, cutoffMethod=PME)
+        system = ff.createSystem(pdb.topology, nonbondedMethod=PME)
 
         # Split forces into groups
         for force in system.getForces():
@@ -160,7 +160,7 @@ class TestIntegrators(unittest.TestCase):
         """Test partial thermostating in the NoseHooverIntegrator (only API)"""
         pdb = PDBFile('systems/alanine-dipeptide-explicit.pdb')
         ff = ForceField('amber99sbildn.xml', 'tip3p.xml')
-        system = ff.createSystem(pdb.topology, cutoffMethod=PME)
+        system = ff.createSystem(pdb.topology, nonbondedMethod=PME)
 
         integrator = NoseHooverIntegrator(1.0*femtosecond) 
         integrator.addSubsystemThermostat(list(range(5)), [], 200*kelvin, 1/picosecond, 200*kelvin, 1/picosecond, 3,3,3)