OpenFreeEnergy / openfe

The Open Free Energy toolkit
https://docs.openfree.energy
MIT License
124 stars 15 forks source link

Non-bonded cutoffs of mainline OpenFF force fields not used #809

Open mattwthompson opened 3 months ago

mattwthompson commented 3 months ago

OpenFF's mainline force fields use a 9 A cutoff for vdW interactions. Code paths involving openmmforcefields do not, by default, use this cutoff and instead fall back to the default of 1 nm:

>>> openmm.NonbondedForce().getCutoffDistance()
Quantity(value=1.0, unit=nanometer)

This can be set in general in OpenMM like

ForceField.createSystem(
    ...,
    nonbondedCutoff=0.9 * openmm.unit.nanometer,
    ...,
)

Unfortunately this is hardcoded in SystemGenerator.create_system which appears to be what this codebase calls. Something like .setCutoffDistance() after system creation should work in either case.

I don't know how much of an impact using 10 A has, but OpenFF's force fields thus far are trained with a 9 A cutoff in condensed-phase fitting and are shipped with that in the non-bonded settings.

Related https://github.com/openmm/openmmforcefields/pull/324

IAlibay commented 3 months ago

Thanks for raising this @mattwthompson !

I don't know how much of an impact using 10 A has, but OpenFF's force fields thus far are trained with a 9 A cutoff in condensed-phase fitting

This, similar to your other issue, is also something that would need testing / validation. I suspect cancellation of error minimizes the impact here but it will need further discussion.

Unfortunately this is hardcoded in SystemGenerator.create_system which appears to be what this codebase calls.

I will have to review this a bit later, but my understanding is that it is possible to set alternative cutoffs via system_generator / the OpenFE Protocol settings (for example see this test: https://github.com/OpenFreeEnergy/openfe/blob/main/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py#L560).

P.S. The link you point to is for DummySystemGenerator, I'm not sure if this is intentional?

mattwthompson commented 3 months ago

Good catch, I'm confused by the different classes and method names. It might get passed through here when called here, not sure if it's set later?

IAlibay commented 3 months ago

We're passing the cutoff on object creation here: https://github.com/OpenFreeEnergy/openfe/blob/main/openfe/protocols/openmm_utils/system_creation.py#L97

Which then gets used within SystemGenerator here: https://github.com/openmm/openmmforcefields/blob/0.12.0/openmmforcefields/generators/system_generators.py#L333

That being said, one thing to note is that we should add some tests for the AFE and MD Protocols that custom cutoffs are used/retained (I'm reasonably sure that they are, but sure != certain).

mattwthompson commented 3 months ago

Ah okay I missed those are passed to that constructor. I'm used to using createSystem