2021-01-08 Sigma/rmin_half setter fix meeting notes

Verification of problem

• Bug seems to have been introduced in #750.

• Basically, the equation for calculating sigma from rmin_half involves a term, r_0, which is twice rmin_half. In recent changes, we’ve routed around the OLD code path that implemented this equation, and the new code that implements it leaves off the factor of , treating rmin_half as if it were r_0

Immediate fix

• JW – Review simon’s PR (https://github.com/openforcefield/openforcefield/pull/808/files), immediately cut a new release.

  • Decision – Jeff and Matt will review+merge PR immediately

• What to do about 0.8.1 and 0.8.2 packages?

  • SB – Could pull them from Anaconda

  • JW – Could put them in off-main label

    • SB – In favor of adding them to a broken label

  • MT – I don’t think there’s any major blocker to making people restart work if they’re using 0.8.1

  • Decision – We will put them on omnia/label/broken

• More energy tests

  • SB – Could use an old version of the toolkit to calculate energies, and then add those as --runslow regression tests

  • (General) – Variety of molecules, a few situations like small molecule dimers.

  • Decision – Before release, we will put in a few energy tests (~5) based on numerical results using older version of toolkit and a single OpenFF OFFXML. In the longer term, we will add a more comprehensive test suite for molecule energies.

  • Spec:

    • Must cover each of the ParameterHandlers in the toolkit’s forcefield class

      • For charges TIP3P and TIP5P library charges

    • Must include tests for some single molecules and some molecule dimers

    • Must include tests that are and are not periodic

    • Reference energies should be calculated using OpenFF toolkit 0.8.0 and openff_unconstrained-1.0.0.offxml and openff-1.0.0.

    • Molecules should be

      • molecules/ethanol.sdf

      • methane_multiconformer.sdf (with two methanes)

      • CID_207…_anion.sdf

      • toluene.sdf

      • water dimer

Long term fix

• JW – Could make a new build with HUGE warning on load

  • MT – Add big warning to top-level init

  • Make two branch starting from the 0.8.1 and 0.8.2 release tags, have one commit on that branch (adding warning), then either make a new tag (eg 0.8.1broken) or create new package build direct from commit to that branch

    • MT – Slightly in favor of tag.

  • SB – I’m in favor of not making any main-label package at all. People who specifically want these packages should have to confirm that there’s something wrong by reading through the justification.

  • SB – If we do add a new pacakge, it shouldn’t raise a warning, it should only raise an error on import.

  • Decision – We will not make any new package at all

• SB – Could add protein-ligand energy regression tests, and tests for other use cases that will likely be of high importance to users.

• Something like Trevor’s virtual sites test?

• After fix, relative energy differences were down to 1e-5. Only one molecule agrees to 1e-7. Why can’t we do better?

• (Discussion paused in the interest of accomplishing short term goals)

Post mortem

• MT – Will this kind of rollback/label shifting be possible on conda-forge?

  • SB – I think so

• SB – Are there other areas of “same variable, multiple names”? Could we come up with a pattern for how to handle these in a unified way so that the conversions are done more safely?

• (Folks have many more thoughts, we’re putting this on hold until a good build is available)