Item | 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. 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 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 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
|
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 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 – 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)
|