openmmtools icon indicating copy to clipboard operation
openmmtools copied to clipboard

Tests failing for nightly OpenMM builds

Open jchodera opened this issue 3 years ago • 5 comments

Tests are failing with nightly OpenMM builds:

======================================================================
FAIL: ContextCache._copy_integrator_state correctly copies state.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/share/miniconda/envs/test/lib/python3.6/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/runner/work/openmmtools/openmmtools/openmmtools/tests/test_cache.py", line 212, in test_copy_integrator_state
    assert integrator1.__getstate__() == integrator2.__getstate__()
AssertionError

jchodera avatar Oct 28 '20 17:10 jchodera

Nightlies (and the incoming 7.5) keep failing. For the record, our code didn't change, but OpenMM did. Exactly at this point:

WORKS (Aug 12, 2020) -- 8bc2ed94604391d71856a33c1b23e21be95c4ec8 FAILS (Aug 18, 2020) -- b4543a46335e7dff5d1c2555d1ec98389e2f4dfb

The diff.

Linux packages for both OpenMM. Disregard the version and hash in the package name, that's an accidental artifact of the quick build I forgot to change. GitHub also forces me to use .zip but it's just a tar.bz2. Remove the extension and run:

conda create -n openmm-works  #empty env
conda activate openmm-works
conda install path.to.tar.bz2
conda update --all

openmm-7.5.0-py37h9166dd1_0--FAILS.tar.bz2.zip openmm-7.5.0-py37h9166dd1_0--WORKS.tar.bz2.zip

jaimergp avatar Dec 08 '20 10:12 jaimergp

The test case that is a problem here is actually the one where both are supposed by at 270K already. The problem is a rounding issue in simtk.unit:

from simtk import unit
from openmmtools.constants import kB

temp = 270.0 * unit.kelvin
print(temp)
print(kB)
# 270.0 K
# 8.31446261815324 J/(K mol)

kT = temp * kB
kT_in_J = kT.value_in_unit(unit.joule / unit.mole)
kT_in_kJ = kT.value_in_unit(unit.kilojoules_per_mole)
print(kT_in_J)
print(kT_in_kJ)
# 2244.9049069013745
# 2.244904906901374

kT_in_J * 0.001 == kT_in_kJ
# False

Tests pass (locally, at least: macOS Python 3.7) after the following (ugly as sin) diff:

diff --git a/openmmtools/integrators.py b/openmmtools/integrators.py
index be419ec..500f273 100644
--- a/openmmtools/integrators.py
+++ b/openmmtools/integrators.py
@@ -221,6 +221,8 @@ class ThermostatedIntegrator(utils.RestorableOpenMMObject, PrettyPrintableIntegr
 
         """
         kT = kB * temperature
+        kT = (kT.value_in_unit(unit.joules/unit.mole) * 0.001
+              * unit.kilojoules_per_mole)
         self.setGlobalVariableByName('kT', kT)
 
         # Update the changed flag if it exist.

I can make a PR of that if desired, though it's an ugly workaround. I didn't dive into the upstream simtk.unit issue, but there seems to be a rounding problem when it calculates conversion_factor_to:

J_per_mol = unit.joules / unit.mole
J_per_mol.conversion_factor_to(unit.kilojoules_per_mole)
# 0.0009999999999999998

(Dug into this because I'm hoping to see openmmtools on conda-forge -- or at least with Py 3.8 and 3.9 -- sometime soon, so OPS can test against those!)

dwhswenson avatar Dec 29 '20 10:12 dwhswenson

Oh, thanks for debugging this issue! I am more or less relieved the issue is only a float arithmetic thing. I'll let the rest of the team comment on this too (we can just add a little margin in the test assertions), but I agree with you that it's better handled upstream. Thanks again!

jaimergp avatar Dec 29 '20 12:12 jaimergp

Btw @dwhswenson openmmtools is on conda-forge for some days now!

jaimergp avatar Feb 16 '21 00:02 jaimergp

https://github.com/openmm/openmm/pull/2968 should have fixed this issue, but I am looking into it at #498 since the nightlies are still failing...

jaimergp avatar Feb 16 '21 00:02 jaimergp