Remove CMMRemover (if present) in SMIRNOFFTemplateGenerator

[j-wags]

• Fixes Interchange 0.4.2 breaks SMIRNOFF template generator #365

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.75%. Comparing base (7565e25) to head (423c488).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #367      +/-   ##
==========================================
+ Coverage   69.57%   69.75%   +0.18%     
==========================================
  Files           5        5              
  Lines         825      830       +5     
==========================================
+ Hits          574      579       +5     
  Misses        251      251              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mattwthompson]

A little safer - hard to expect multiple CMM removers, but this is modifying a list while iterating over it. Not sure what happens if, for example, the force is listed last

Suggested change

	system.removeForce(f_idx)
	break
	system.removeForce(f_idx)

[peastman]

Loop in reverse order from system.getNumForces()-1 down to 0. That way the indices will still be correct if you remove one.

The best way to check the force type is if isinstance(force, CMMotionRemover).

Why are you removing CMMotionRemovers?

[mattwthompson]

The check here flags it as a custom force:

openmmforcefields/openmmforcefields/generators/template_generators.py

Lines 1116 to 1131 in 7a32d2d

	supported_forces = {
	"NonbondedForce",
	"HarmonicAngleForce",
	"HarmonicBondForce",
	"PeriodicTorsionForce",
	}
	# Compile forces into a dict
	forces = dict()
	for force in system.getForces():
	force_name = force.__class__.__name__
	if force_name in forces:
	raise ForceException(f"Two instances of force {force_name} appear in System")
	if force_name not in supported_forces:
	raise ForceException(f"Custom forces not supported. Found force of type {force_name}.")

[peastman]

That means you should fix the check, not remove the force. The default for createSystem() is removeCMMotion=True. This change would break it.

[mattwthompson]

This check happens when generating the residue template from OpenFF tooling, not the system that's ultimately passed to the user

[mikemhenry]

I would need to look at the code more closely, but what happens if when iterating over system.getForces() you just continue if the force is CMMotionRemover? That should keep it out of the forces dict?

[mikemhenry]

Okay so if we go with my idea, CMMotionRemover won't end up in the ffxml when you call generate_residue_template -- is that a problem? If it is, then all we need to do is write CMMotionRemover into the ffxml

[j-wags]

We're considering two approaches:

1. (the initial approach of this PR) Delete CMMotionRemover from OpenFF-generated systems during template generation
2. ("@mikemhenry's approach") Skip over CMMotionRemovers in OpenMMSystemMixin.convert_system_to_ffxml

I've tried both of these locally and approach 1 seems like a better idea, since

• the new behavior is narrowly applied only to OpenFF-generated systems rather than affecting all mixins
• approach 2 also requires changing a test of all template generators since it also compared the Force objects on two generated systems.

@peastman Thanks for the code tips, I'll add those.

Why are you removing CMMotionRemovers?

The CMMotionRemover started getting added to OpenFF-generated OpenMM systems in the Interchange 0.4.2 release yesterday. This broke SMIRNOFFTemplateGenerator since it wasn't expecting the new Force when creating residue templates.

The default for createSystem() is removeCMMotion=True.

My understanding is that the change proposed in this PR does not interact with that behavior. This is just cleaning up the generation of templates. The actual inclusion of the CMMotionRemover in the final system is still dependent on the removeCMMotion argument to createSystem .

If there are no objections, I'll advance this PR along the lines of approach 1.

[mattwthompson]

Strong preference for 1

[epretti]

Looks good to me. Since Matt has also approved, I will go ahead and merge.