Pymbar 3 4 #659

mikemhenry · 2023-03-10T17:18:15Z

Description

Provide a brief description of the PR's purpose here.

Todos

Implement feature / fix bug
Add tests
Update documentation as needed
Update changelog to summarize changes in behavior, enhancements, and bugfixes implemented in this PR

Status

Ready to go

Changelog message

Co-authored-by: Iván Pulido <[email protected]>

mikemhenry · 2023-03-10T17:18:44Z

Supersedes #630

mikemhenry · 2023-03-14T01:55:33Z

hmmmm, seeing the same errors for pymbar 3&4


======================================================================
ERROR: Test multistate sampler on a harmonic oscillator without unsampled endstates
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/micromamba-root/envs/openmmtools-test/lib/python3.8/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/runner/work/openmmtools/openmmtools/openmmtools/tests/test_sampling.py", line 234, in test_without_unsampled_states
    self.run(include_unsampled_states=False)
  File "/home/runner/work/openmmtools/openmmtools/openmmtools/tests/test_sampling.py", line 223, in run
    raise Exception("Dimensionless free energy difference exceeds MAX_SIGMA of %.1f" % MAX_SIGMA)
Exception: Dimensionless free energy difference exceeds MAX_SIGMA of 6.0

mikemhenry · 2023-03-14T02:33:19Z

That test passes fine on main but on this branch it fails ~10% of the time...

mikemhenry · 2023-03-14T23:57:13Z

I yelled when I saw this: https://github.com/choderalab/openmmtools/blob/main/openmmtools/multistate/utils.py#L178-L183

I could not figure out how an import fixed things, well it "solved" the problem that this bare except hid.

BUT what is crazier is that we don't get a failure every time, but will essentially calculate a subsample rate larger than than our input array, which causes our equilibrated indices to be indices=[0] which sometimes passes our tests if we are lucky...

Anyway, bare excepts were a bad idea. I can't belive the amount of time I spent on this.

…n so we can figure out what we are trying to catch

mikemhenry · 2023-03-15T00:28:57Z

I will be cleaning up those exceptions in this PR as well....

ijpulidos · 2023-03-15T00:30:48Z

Oh yeah, of course bare excepts are biting us, good catch! Thanks.

…ymbar-3-4

openmmtools/tests/test_cache.py

openmmtools/multistate/multistatereporter.py

mikemhenry · 2023-03-15T09:03:35Z

On windows-latest, py-3.8.10, OpenMM-8.0, pymbar-4

======================================================================
ERROR: Test expected number of entries in real time analysis output yaml file.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\runneradmin\micromamba-root\envs\openmmtools-test\lib\site-packages\nose\case.py", line 197, in runTest
    self.test(*self.arg)
  File "D:\a\openmmtools\openmmtools\openmmtools\tests\test_sampling.py", line 1577, in test_real_time_analysis_yaml
    sampler.run()
  File "D:\a\openmmtools\openmmtools\openmmtools\multistate\multistatesampler.py", line 791, in run
    self._update_timing(iteration_time, partial_total_time, run_initial_iteration, iteration_limit)
  File "D:\a\openmmtools\openmmtools\openmmtools\multistate\multistatesampler.py", line 1773, in _update_timing
    self._timing_data["ns_per_day"] = iteration_simulated_nanoseconds / (
ZeroDivisionError: float division by zero

and on windows-latest, py-3.10, OpenMM-8.0, pymbar-3

======================================================================
ERROR: Test that creating a MultiState by storage string and reporter is the same
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\runneradmin\micromamba-root\envs\openmmtools-test\lib\site-packages\nose\case.py", line 197, in runTest
    self.test(*self.arg)
  File "D:\a\openmmtools\openmmtools\openmmtools\tests\test_sampling.py", line 1436, in test_storage_reporter_and_string
    sampler.run()
  File "D:\a\openmmtools\openmmtools\openmmtools\multistate\multistatesampler.py", line 791, in run
    self._update_timing(iteration_time, partial_total_time, run_initial_iteration, iteration_limit)
  File "D:\a\openmmtools\openmmtools\openmmtools\multistate\multistatesampler.py", line 1773, in _update_timing
    self._timing_data["ns_per_day"] = iteration_simulated_nanoseconds / (
ZeroDivisionError: float division by zero

All the other windows builds are fine, but we probably do want to catch that error so it doesn't crash a simulation if it happens. That can be done in another PR.

mikemhenry · 2023-03-15T15:24:52Z

Okay! Since everything is passing except those two errors, I am now going to switch up the CI matrix so that we are testing mostly with pymbar 4, but will keep a few jobs at pymbar 3 to make sure we maintain support for both.

mikemhenry · 2023-03-15T15:48:39Z

Once we finalize the matrix, I will mark them as required

ijpulidos

This is looking great. Just a few comments I think we need to adress.

openmmtools/multistate/multistatesampler.py

ijpulidos · 2023-03-16T04:15:02Z

openmmtools/multistate/utils.py

+            print("If you see this, open an issue https://github.com/choderalab/openmmtools/issues")
+            raise e


Do we want to raise the exception here? I feel like this might be disruptive for current users/applications.

See #659 (comment) I think we can see if perses has any problems as well as other downstream packages as a double check. I worry it does more damage leaving it in the codebase. I could be convinced to just log an error but I think no one will see it.

ijpulidos

Looks great, thanks! I tested with perses locally in a tyk2 benchmark system and it seems to be working (using pymbar3). LGTM!

openmmtools/multistate/multistatesampler.py

mattwthompson and others added 10 commits November 21, 2022 14:41

Make compatible with pymbar 3 and 4

6a2a437

Try a different syntax

2d9e21f

Fix syntax

8d36afe

Merge branch 'main' into pymbar-3-4

29c37ba

Update openmmtools/multistate/multistateanalyzer.py

dc1d0d6

Co-authored-by: Iván Pulido <[email protected]>

fix pymbar install step

0c35588

Update .gitignore

e4b21fc

Merge remote-tracking branch 'origin/pymbar-3-4' into pymbar-3-4

107314a

Update workflow

21b1696

Merge branch 'main' into pymbar-3-4

fcd2cfe

mikemhenry added 5 commits March 13, 2023 16:22

Merge branch 'main' into pymbar-3-4

a976d7b

Merge branch 'main' into pymbar-3-4

e8284d3

set env name to match file

a151b48

fix package spec

29726a7

remove pymbar pin in envs

60c5a25

mikemhenry added 3 commits March 14, 2023 10:19

Remove unused import

abddeab

🧙magic import fixes pymbar 3 🧙

96f0f2a

fix matrix

d474bd1

mikemhenry added 5 commits March 14, 2023 17:08

fix missing statistical_inefficiency conversion and raise an exceptio…

d393000

…n so we can figure out what we are trying to catch

missed a doc string

30988ef

forgot to actually change the name

9f5dbe8

add note so we can figure out when this error can happen in the wild

65688a0

lets try and see what excpetions we might be throwing

c74d34d

catught a few more api changes

8a9c867

mikemhenry and others added 4 commits March 14, 2023 18:03

consolidate all pymbar imports now

1c341e8

Support for both pymbar 3 and 4 in test function

08c3ff3

Merge branch 'pymbar-3-4' of github.com:choderalab/openmmtools into p…

59677e3

…ymbar-3-4

Merge branch 'main' into pymbar-3-4

a47c886

ijpulidos reviewed Mar 15, 2023

View reviewed changes

openmmtools/tests/test_cache.py Outdated Show resolved Hide resolved

looks like they should be attribute errors

bd72c24

ijpulidos reviewed Mar 15, 2023

View reviewed changes

openmmtools/multistate/multistatereporter.py Outdated Show resolved Hide resolved

log excpetion thrown

5c77b50

mikemhenry requested a review from ijpulidos March 15, 2023 06:51

mikemhenry added 3 commits March 15, 2023 00:23

wrap EXP/exp in function to handle different return types

83e0cd1

fix syntax error on raising exception

8d5d840

fix typo in function

0377477

trim down matrix and update actions/checkout to v3

c227bd1

mikemhenry and others added 3 commits March 15, 2023 14:51

see if this magicly fixes the mac builds

0b49bf6

looks like the problem is elsewhere

8014c35

Pinning hdf5 version. Possible fix for macOS CI.

1cdf490

ijpulidos requested changes Mar 16, 2023

View reviewed changes

mikemhenry requested a review from ijpulidos March 16, 2023 22:03

ijpulidos approved these changes Mar 17, 2023

View reviewed changes

openmmtools/multistate/multistatesampler.py Show resolved Hide resolved

ijpulidos added this to the 0.22.0 milestone Mar 17, 2023

mikemhenry merged commit aaa4719 into main Mar 17, 2023

mikemhenry deleted the pymbar-3-4 branch March 17, 2023 02:30

ijpulidos mentioned this pull request Mar 20, 2023

Make compatible with pymbar 3 and 4 #630

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pymbar 3 4 #659

Pymbar 3 4 #659

mikemhenry commented Mar 10, 2023

mikemhenry commented Mar 10, 2023

mikemhenry commented Mar 14, 2023

mikemhenry commented Mar 14, 2023

mikemhenry commented Mar 14, 2023

mikemhenry commented Mar 15, 2023

ijpulidos commented Mar 15, 2023

mikemhenry commented Mar 15, 2023

mikemhenry commented Mar 15, 2023

mikemhenry commented Mar 15, 2023

ijpulidos left a comment

ijpulidos Mar 16, 2023

mikemhenry Mar 16, 2023

ijpulidos left a comment

		print("If you see this, open an issue https://github.com/choderalab/openmmtools/issues")
		raise e

Pymbar 3 4 #659

Pymbar 3 4 #659

Conversation

mikemhenry commented Mar 10, 2023

Description

Todos

Status

Changelog message

mikemhenry commented Mar 10, 2023

mikemhenry commented Mar 14, 2023

mikemhenry commented Mar 14, 2023

mikemhenry commented Mar 14, 2023

mikemhenry commented Mar 15, 2023

ijpulidos commented Mar 15, 2023

mikemhenry commented Mar 15, 2023

mikemhenry commented Mar 15, 2023

mikemhenry commented Mar 15, 2023

ijpulidos left a comment

Choose a reason for hiding this comment

ijpulidos Mar 16, 2023

Choose a reason for hiding this comment

mikemhenry Mar 16, 2023

Choose a reason for hiding this comment

ijpulidos left a comment

Choose a reason for hiding this comment