Update HybridCompatibilityMixin to handle RESTCapableHybridTopologyFactory and add repex self-consistency tests #992

zhang-ivy · 2022-05-03T23:22:20Z

Description

Updated HybridCompatilityMixin to automatically detect which type of factory (HybridTopologyFactory or RESTCapableHybridTopologyFactory) was inputted and create a sampler depending on the factory type.
Added self-consistency tests for dipeptide systems (running repex with the new RESTCapableHybridTopologyFactory):
- Neutral mutation: ALA->THR vs THR->ALA
- Charge changing: ARG->ALA->ARG vs LYS->ALA->LYS (see docstring of test for explanation on why these particular transformations are necessary)

TODO:

I put the tests in test_samplers.py -- not sure if this is the right place -- may want to move it somewhere else and/or make it an example?
Not sure if the tests in test_samplers.py are even working -- should we delete everything else in here?
n_iterations should be bumped up to 1000 and it should be run on a gpu (@mikemhenry : Can you help with this?)

Motivation and context

Resolves #984

How has this been tested?

Change log

mikemhenry · 2022-05-04T07:50:44Z

Could we run a few iterations on the CPU but skip the validation checks, and run the 1000 iterations on the GPU? An advantage of just doing a few iterations on the CPU is it will help find API/other run time errors faster

zhang-ivy · 2022-05-04T11:34:25Z

Yes that sounds good. Sorry I wasn’t clear, that’s actually what I meant. On May 4, 2022, at 03:51, Mike Henry ***@***.***> wrote: Could we run a few iterations on the CPU but skip the validation checks, and run the 1000 iterations on the GPU? An advantage of just doing a few iterations on the CPU is it will help find API/other run time errors faster — Reply to this email directly, view it on GitHub<#992 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AIPGJCSQ5JGHJHIY6KPYFA3VIIT57ANCNFSM5VALN3DQ>. You are receiving this because you authored the thread.Message ID: ***@***.***>

ijpulidos

Looks good! Just a few comments.

On the other hand, in my opinion these tests should be examples, and we are already testing the examples in test_examples.py, there's probably a bit of tweaking to be done in order to get this run in the new GPU CI, since the examples predates it. These tests are not really testing "units" but rather complete workflows/simulations. I can make the changes for this to be converted as examples, we also need examples with repex.

perses/tests/test_samplers.py

Co-authored-by: Iván Pulido <ivanpulido@protonmail.com>

jchodera · 2022-05-08T02:24:34Z

Can this make it into the 0.10.0 release, or will it have to wait until 0.11.0?

zhang-ivy · 2022-05-08T13:17:28Z

@jchodera : This can probably make it into 0.10.0, we just need to decide whether we want to keep the tests I wrote as tests or convert them to examples. Was thinking we could discuss this tomorrow at the dev sync.

After we make a decision, we might need @mikemhenry to help specify which tests should be run on cpu vs gpu.

zhang-ivy · 2022-05-08T13:20:19Z

I should also note that the new tests I added in this PR are the reason why the GHA workflow has failures. This is because the tests are only running 2 cycles of repex, which is surely not enough to get the free energies to be equal and opposite. We'll need to move this test to the gpu to get the free energies to match.

zhang-ivy · 2022-05-09T20:40:55Z

Notes from dev sync:

We need to determine the smallest number of replicas and iterations necessary to get the repex examples to converge.
Make this an example (not a test) -- which will be marked as gpu only and will run this once a week on aws (or lilac??) to catch potential regressions.

Co-authored-by: Iván Pulido <ivanpulido@protonmail.com>

ijpulidos · 2022-05-25T00:04:38Z

I keep getting the following error traceback when running the counterion mutation test using 4 or more iterations (2 work):

Traceback (most recent call last):
  File "/lila/data/chodera/pulidoi/sandbox/perses-992/test_REST_counterion_mutation.py", line 79, in <module>
    f_ij, df_ij = analyzer.get_free_energy()
  File "/home/pulidoi/miniconda3/envs/perses-dev/lib/python3.9/site-packages/openmmtools/multistate/multistateanalyzer.py", line 1932, in get_free_energy
    self._compute_free_energy()
  File "/home/pulidoi/miniconda3/envs/perses-dev/lib/python3.9/site-packages/openmmtools/multistate/multistateanalyzer.py", line 1892, in _compute_free_energy
    (Deltaf_ij, dDeltaf_ij) = self.mbar.getFreeEnergyDifferences()
  File "/home/pulidoi/miniconda3/envs/perses-dev/lib/python3.9/site-packages/pymbar/mbar.py", line 541, in getFreeEnergyDifferences
    Theta_ij = self._computeAsymptoticCovarianceMatrix(
  File "/home/pulidoi/miniconda3/envs/perses-dev/lib/python3.9/site-packages/pymbar/mbar.py", line 1679, in _computeAsymptoticCovarianceMatrix
    check_w_normalized(W, N_k)
  File "/home/pulidoi/miniconda3/envs/perses-dev/lib/python3.9/site-packages/pymbar/utils.py", line 358, in check_w_normalized
    raise ParameterError(
pymbar.utils.ParameterError: Warning: Should have \sum_n W_nk = 1.  Actual column sum for state 0 was 0.635215. 12 other columns have similar problems

zhang-ivy · 2022-05-25T00:06:48Z

Ah yes that’s an issue with your version of pymbar. Make sure you install from master. On May 24, 2022, at 20:04, Iván Pulido ***@***.***> wrote: I keep getting the following error traceback when running the counterion mutation test using 4 or more iterations (2 work): Traceback (most recent call last): File "/lila/data/chodera/pulidoi/sandbox/perses-992/test_REST_counterion_mutation.py", line 79, in <module> f_ij, df_ij = analyzer.get_free_energy() File "/home/pulidoi/miniconda3/envs/perses-dev/lib/python3.9/site-packages/openmmtools/multistate/multistateanalyzer.py", line 1932, in get_free_energy self._compute_free_energy() File "/home/pulidoi/miniconda3/envs/perses-dev/lib/python3.9/site-packages/openmmtools/multistate/multistateanalyzer.py", line 1892, in _compute_free_energy (Deltaf_ij, dDeltaf_ij) = self.mbar.getFreeEnergyDifferences() File "/home/pulidoi/miniconda3/envs/perses-dev/lib/python3.9/site-packages/pymbar/mbar.py", line 541, in getFreeEnergyDifferences Theta_ij = self._computeAsymptoticCovarianceMatrix( File "/home/pulidoi/miniconda3/envs/perses-dev/lib/python3.9/site-packages/pymbar/mbar.py", line 1679, in _computeAsymptoticCovarianceMatrix check_w_normalized(W, N_k) File "/home/pulidoi/miniconda3/envs/perses-dev/lib/python3.9/site-packages/pymbar/utils.py", line 358, in check_w_normalized raise ParameterError( pymbar.utils.ParameterError: Warning: Should have \sum_n W_nk = 1. Actual column sum for state 0 was 0.635215. 12 other columns have similar problems — Reply to this email directly, view it on GitHub<#992 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AIPGJCVM65SVZ3DD5UM4GNDVLVVCDANCNFSM5VALN3DQ>. You are receiving this because you authored the thread.Message ID: ***@***.***>

zhang-ivy · 2022-05-25T13:21:01Z

@ijpulidos : This is the pymbar issue related to your error: choderalab/pymbar#419

zhang-ivy · 2022-05-25T13:22:25Z

@ijpulidos : Actually, which version are you using? 3.0.6+ should be fine (i.e. you don't necessarily need to install from master), 3.0.5 is broken.

ijpulidos · 2022-05-25T14:53:09Z

@zhang-ivy yes, I was using 3.0.5, I can confirm 3.0.6+ works fine. Thanks!

I checked the behavior of the free energy with the number of iterations. I don't see any tendency or convergence. The results are as follows

# neutral mutation
n_iterations, energy_diff
2,      4.18895
4,      0.990343
8,      -0.00456955
16,     -5.50846
32,     -0.54498
64,      -1.93432
128,     0.827766
256,     0.176885
512,     0.0994963  # took ~3 hours

# counterion mutation
n_iterations, energy_diff
2,      4.79188
4,      6.45721
8,      0.626254
16,     1.82293
32,     -1.89355
64,     1.7545
128,    -2.88888   # took ~1.5 hours

None of them converged (they all resulted in an AssertionError as per the test). The walltime for the longest ones are specified.

zhang-ivy · 2022-05-25T15:03:13Z

@ijpulidos : I think the n_iterations is too small, can you try n_iterations=250, 500, 1000, 1500, 2000? You basically already did the first 2 for the neutral mutation, so no need to repeat those.

ijpulidos · 2022-05-25T15:06:31Z

@zhang-ivy Sure I can try, I'm just wondering if we are okay with tests running for more than 6 hours in the GPU CI. I guess it's not much of an issue if these are running overnight or so.

zhang-ivy · 2022-05-25T15:16:52Z

@ijpulidos : Yes, I'm not sure either. We can discuss more (with Mike, too) once we determine what the minimal n_iterations is.

mikemhenry · 2022-05-26T17:16:37Z

Yah lets first figure out what the minimal amount of time we can spend is, then we can figure out what we want to do with that. It may be something we check before a release or once a month or something. A p2.xl is something like 90 cents an hour, so even it it takes 10 hours, that isn't munch per month or week if we decide we need it.

ijpulidos · 2022-05-26T18:59:35Z

The most recent results are as follows (I'm including previous results as well just in case we spot some pattern or something).

# neutral mutation
n_iterations, energy_diff
2,      4.18895
4,      0.990343
8,      -0.00456955
16,     -5.50846
32,     -0.54498
64,      -1.93432
128,     0.827766
256,     0.176885
512,     0.0994963  # took ~3 hours
1024,   -0.199971
1512,   -0.026376  # took ~13 hours in A40

# counterion mutation
n_iterations, energy_diff
2,      4.79188
4,      6.45721
8,      0.626254
16,     1.82293
32,     -1.89355
64,     1.7545
128,    -2.88888   # took ~1.5 hours
512,    1.87339
1024,   -1.04253
1512,   -1.37579
2048,   0.446562
4096,   -0.247799
5000,   -0.436782  # took ~17 hours in A100

These are still not converged in terms of np.isclose which, as far as I can see, defaults to checking around 5 significant figures. I wonder if we are expecting too much and whether we should change our convergence criteria. As far as I can see, it also depends on the system, the counterion one is one order of magnitude further apart compared to the neutral system. Thoughts?

ijpulidos · 2022-05-26T19:08:03Z

As far as I can see, it also depends on the system, the counterion one is one order of magnitude further apart compared to the neutral system. Thoughts?

Actually, nvm this part, if we check the significant figures in the actual values (not the difference) it is similar for both systems. For example:

# neutral system
ALA-THR is -51.84165478283243. THR-ALA is -51.81527874133261
# counterion system
ARG-ALA-ARG is -277.7522884618811. LYS-ALA-LYS is -277.31550615053516

zhang-ivy · 2022-05-26T19:22:27Z

I wonder if we are expecting too much and whether we should change our convergence criteria.

Yes, I think the convergence criteria is too stringent. Let's just check whether the difference in the forward and reverse free energies is < 1 kT, or perhaps 0.5 kT if we want to be a bit more stringent.

Seems like for the neutral mutations, 250 iterations should be sufficient. For the charge changing, seems like 2000 should be good.

ijpulidos · 2022-05-27T20:36:27Z

Ok, so I think we leave these as tests (now I think they are actually tests not examples, sorry for my confusion before). I'll be pushing the changes soon.

@mikemhenry I'm wondering if we use a new pytest mark for these kind of tests that could potentially take hours/days maybe having yet another CI workflow that run these specially marked tests every once in a while or so? Ideas on how could we accomplish this are appreciated.

…r own module.

codecov · 2022-05-27T21:12:32Z

Codecov Report

Merging #992 (af2862c) into main (ef35201) will decrease coverage by 0.24%.
The diff coverage is 15.59%.

mikemhenry · 2022-05-31T20:29:44Z

@ijpulidos How about we merge this in and see how long it takes on AWS? How long does it take if we only care about <1kT?

ijpulidos · 2022-05-31T23:58:12Z

@ijpulidos How about we merge this in and see how long it takes on AWS? How long does it take if we only care about <1kT?

So there are two tests, the first one might run in a reasonable time, the second one I have the suspicion that it is going to take more than 24 hours to run on the current AWS instance we are using. I guess we can just try and see, I'm going to make the changes for this to be run (it's now explicitly skipped with pytest). What's a good limit for this? My assumption is that anything below 24 hours should be okay since we are running the GPU CI every 24 hours as well, but I don't know if there are other limits (github/aws limiting it).

mikemhenry · 2022-06-01T00:15:28Z

On a self hosted runner I think it's like a 30 day timeout 🤣 let's do it an see what happens

zhang-ivy · 2022-06-01T13:50:41Z

@ijpulidos : I just reviewed your changes and I don't think we should call the file test_topology_factories.py -- we aren't actually testing the factory here -- we do that in test_relative.py, which contains the energy validation tests for the factories -- I think we should leave it as test_samplers.py, since we are checking to make sure the sampler HybridCompatabilityMixin is working properly.

Or, perhaps an even better name for the test file might be test_repex_convergence.py or test_repex_self_consistency.py

PS: We'll want to put the small molecule repex test in this same file as well, see: #959

ijpulidos · 2022-06-01T18:37:52Z

@zhang-ivy Yes, I agree. I think it makes sense to have a test_repex.py module or similar with all the repex tests (these ones at least, maybe we'll migrate/create others with time). Similarly with others like sams and neq-switching, etc. I'll make the changes.

zhang-ivy · 2022-06-01T19:49:03Z

@ijpulidos : Tests are passing -- as long as you specified the right flags for running the tests on GPU, I think this should be good to merge!

mikemhenry

LGTM

update repex sampler and add tests

58c26f5

zhang-ivy requested review from mikemhenry, jchodera and ijpulidos May 3, 2022 23:22

zhang-ivy changed the title ~~Update repex sampler and add tests~~ Update HybridCompatibilityMixin to handle RESTCapableHybridTopologyFactory and add repex self-consistency tests May 3, 2022

ijpulidos requested changes May 4, 2022

View reviewed changes

perses/tests/test_samplers.py Outdated Show resolved Hide resolved

perses/tests/test_samplers.py Outdated Show resolved Hide resolved

perses/tests/test_samplers.py Outdated Show resolved Hide resolved

zhang-ivy and others added 2 commits May 6, 2022 11:10

fix platform 1

af9c995

Co-authored-by: Iván Pulido <ivanpulido@protonmail.com>

fix platform 2

df30671

Co-authored-by: Iván Pulido <ivanpulido@protonmail.com>

jchodera added this to the 0.10.0 Enhanced CLI with more stable input/output formats milestone May 8, 2022

jchodera added the priority: medium priority medium label May 8, 2022

mikemhenry and others added 2 commits May 23, 2022 12:32

Merge branch 'main' into update-sampler

06a7df2

Update perses/tests/test_samplers.py

0127b3e

Co-authored-by: Iván Pulido <ivanpulido@protonmail.com>

ijpulidos added 2 commits May 27, 2022 16:53

Specifying iterations and convergence tolerance. Moving tests to thei…

703e818

…r own module.

Merge branch 'main' into update-sampler

b810e54

ijpulidos added 2 commits May 31, 2022 20:06

Enabling test to check with GPU CI workflow.

36aae93

Merge branch 'main' into update-sampler

505c1ef

ijpulidos self-requested a review June 1, 2022 00:10

Trying to fix doc build.

1e25dde

ijpulidos added 2 commits June 1, 2022 14:41

Tests in repex test module.

bcb5aca

Merge branch 'main' into update-sampler

628673b

mikemhenry approved these changes Jun 1, 2022

View reviewed changes

ijpulidos added 2 commits June 2, 2022 10:46

Merge branch 'main' into update-sampler

5217da1

Merge branch 'main' into update-sampler

af2862c

ijpulidos approved these changes Jun 2, 2022

View reviewed changes

ijpulidos merged commit 41a3cae into main Jun 2, 2022

ijpulidos deleted the update-sampler branch June 2, 2022 15:12

zhang-ivy mentioned this pull request Jun 14, 2022

Fix GPU tests for repex internal consistency (protein mutations) #1044

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update HybridCompatibilityMixin to handle RESTCapableHybridTopologyFactory and add repex self-consistency tests #992

Update HybridCompatibilityMixin to handle RESTCapableHybridTopologyFactory and add repex self-consistency tests #992

zhang-ivy commented May 3, 2022

mikemhenry commented May 4, 2022

zhang-ivy commented May 4, 2022 via email

ijpulidos left a comment

jchodera commented May 8, 2022

zhang-ivy commented May 8, 2022

zhang-ivy commented May 8, 2022

zhang-ivy commented May 9, 2022

ijpulidos commented May 25, 2022

zhang-ivy commented May 25, 2022 via email

zhang-ivy commented May 25, 2022

zhang-ivy commented May 25, 2022 •

edited

Loading

ijpulidos commented May 25, 2022

zhang-ivy commented May 25, 2022 •

edited

Loading

ijpulidos commented May 25, 2022

zhang-ivy commented May 25, 2022

mikemhenry commented May 26, 2022

ijpulidos commented May 26, 2022 •

edited

Loading

ijpulidos commented May 26, 2022 •

edited by zhang-ivy

Loading

zhang-ivy commented May 26, 2022 •

edited

Loading

ijpulidos commented May 27, 2022

codecov bot commented May 27, 2022 •

edited

Loading

mikemhenry commented May 31, 2022

ijpulidos commented May 31, 2022

mikemhenry commented Jun 1, 2022

zhang-ivy commented Jun 1, 2022 •

edited

Loading

ijpulidos commented Jun 1, 2022

zhang-ivy commented Jun 1, 2022

mikemhenry left a comment

Update HybridCompatibilityMixin to handle RESTCapableHybridTopologyFactory and add repex self-consistency tests #992

Update HybridCompatibilityMixin to handle RESTCapableHybridTopologyFactory and add repex self-consistency tests #992

Conversation

zhang-ivy commented May 3, 2022

Description

Motivation and context

How has this been tested?

Change log

mikemhenry commented May 4, 2022

zhang-ivy commented May 4, 2022 via email

ijpulidos left a comment

Choose a reason for hiding this comment

jchodera commented May 8, 2022

zhang-ivy commented May 8, 2022

zhang-ivy commented May 8, 2022

zhang-ivy commented May 9, 2022

ijpulidos commented May 25, 2022

zhang-ivy commented May 25, 2022 via email

zhang-ivy commented May 25, 2022

zhang-ivy commented May 25, 2022 • edited Loading

ijpulidos commented May 25, 2022

zhang-ivy commented May 25, 2022 • edited Loading

ijpulidos commented May 25, 2022

zhang-ivy commented May 25, 2022

mikemhenry commented May 26, 2022

ijpulidos commented May 26, 2022 • edited Loading

ijpulidos commented May 26, 2022 • edited by zhang-ivy Loading

zhang-ivy commented May 26, 2022 • edited Loading

ijpulidos commented May 27, 2022

codecov bot commented May 27, 2022 • edited Loading

Codecov Report

mikemhenry commented May 31, 2022

ijpulidos commented May 31, 2022

mikemhenry commented Jun 1, 2022

zhang-ivy commented Jun 1, 2022 • edited Loading

ijpulidos commented Jun 1, 2022

zhang-ivy commented Jun 1, 2022

mikemhenry left a comment

Choose a reason for hiding this comment

zhang-ivy commented May 25, 2022 •

edited

Loading

zhang-ivy commented May 25, 2022 •

edited

Loading

ijpulidos commented May 26, 2022 •

edited

Loading

ijpulidos commented May 26, 2022 •

edited by zhang-ivy

Loading

zhang-ivy commented May 26, 2022 •

edited

Loading

codecov bot commented May 27, 2022 •

edited

Loading

zhang-ivy commented Jun 1, 2022 •

edited

Loading