⚠️ 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 71.41%. Comparing base (37bec91) to head (6162834).

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

@@           Coverage Diff           @@
##             main     #370   +/-   ##
=======================================
  Coverage   71.41%   71.41%           
=======================================
  Files           5        5           
  Lines         822      822           
=======================================
  Hits          587      587           
  Misses        235      235           

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

Encouraged to see the expected failure: https://github.com/openmm/openmmforcefields/actions/runs/13993907851/job/39184133882?pr=370#step:6:499

I found you may also need to add another exception to exclude from the list of forcefields. spce should be excluded along with tip and opc in TestSMIRNOFFTemplateGenerator.test_energies (and maybe TestGAFFTemplateGenerator.test_parameterize too?). I guess it was added at some point. spce was excluded from only 1 of the 3 places where water models are checked and excluded. Might be good to have a list at the top of the test file of the strings to check instead of duplicating.

Looks like there are some other actual failures in there in terms of energies not matching up:
https://github.com/openmm/openmmforcefields/actions/runs/13994740439/job/39186960028?pr=370#step:6:6353
I can take a look unless you're already digging into it.

So just taking a look at the one molecule linked above that is failing, [H][S][C](=[S])[N]([C]([H])([H])[C]([H])([H])[H])[C]([H])([H])[C]([H])([H])[H], the conformer generated by the OpenFF toolkit is far from the equilibrium geometry predicted by smirnoff99Frosst-1.0.2. The large forces cause the molecule to disintegrate when dynamics are run, so one gets extremely large energy values. They then still match very well between the reference and test systems, but the test is absolute energy difference instead of relative, so it fails.

This could be fixed by changing the energy difference check

The minimized geometry for this molecule looks extremely suspect to me. The root issue for this particular molecule is that this force field is assigning a 4 angstrom (!) bond length.

Heh, I was just in the process of tidying up a small number of details in that file before passing it off to you for a more serious analysis. I hoped to find an easy way to get compare_energies to report more information but found it wasn't something I could do very quickly. Anyway -

I think I speak for the entirety of the initiative in saying that smirnoff99Frosst was a essential stepping stone in the development of our force fields but was never meant to be used in perpetuity. Sage force fields mostly (but not completely) replace Parsley force fields which themselves mostly replaced smirnoff99Frosst. Skipping over some details, there's just no reason for somebody to use it as a "starting point" force field in the middle of 2025.

I don't think we should necessarily make it hard for people to use it in downstream packages like this, but it's also not worth the trouble of digging deep into its particular issues or putting a ton of CI time into testing it. I suggest skipping over it either in tests (my preference) or altogether.

Happy to take a look at it whenever you're ready. Thanks for the note on the context of the development of smirnoff99Frosst.

Seeing failures with (only) this molecule, so I'm looking to see if it's something that only happens with a particular force field

Okay, the failure is with 2.2.1RC1 but it's the same as the released force field:

$ diff ~/software/openff-forcefields/openforcefields/offxml/openff-2.2.1-rc1.offxml ~/software/openff-forcefields/openforcefields/offxml/openff-2.2.1.offxml
4c4
<     <Date>2024-07-22</Date>
---
>     <Date>2024-09-11</Date>

@epretti what should we do here?

OK, I can reproduce, and I'm seeing this with a few force fields. It's the same kind of problem as above with the other molecule I looked at. In this case I think nothing's wrong with the force field, but the generated conformer is bad. A close contact between two atoms creates large forces, the dynamics become unstable, and the resulting configuration can have extremely large energies. Though they match to high precision in the test, the absolute value of the difference can be large.

I think the best way to handle this (that appears to fix the problem for me) is to just minimize before running dynamics; changing

context.setPositions(molecule.conformers[0].to_openmm()) 
openmm.LocalEnergyMinimizer.minimize(context)
integrator.step(nsteps) 

@epretti I think this is mostly ready for you to take over

This should be working now. I updated the troublesome GAFF partial charge test that we discussed recently to just create example user-defined charges that sum to the specified amount. I'm creating a separate issue for what to do about this problem in general, and if it eventually leads to a PR, I can add separate tests there.