-
Notifications
You must be signed in to change notification settings - Fork 75
Update pymbar #317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Update pymbar #317
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
dc168bc
add mbar
lilyminium c020c26
use pymbar as dependency
lilyminium f3421a4
update comment
lilyminium e544fcc
refactor for simplicity
lilyminium 7ff0064
switch to robust solver
lilyminium 2257bc2
replace maxdiff with allclose
lilyminium 1a24b45
add a small atol for really small things
lilyminium d7750f4
update with atol
lilyminium f50ba09
tmp disable jax to check timings
lilyminium 5a642ed
Revert "tmp disable jax to check timings"
lilyminium File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,7 @@ dependencies: | |
| - zlib | ||
| - swig | ||
| - future | ||
| - pymbar =3 | ||
| - pymbar | ||
| - openmm >= 8 | ||
| - ambertools | ||
| - ndcctools | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file not shown.
Binary file not shown.
Binary file not shown.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,13 +2,63 @@ | |
| import os | ||
| import sys | ||
| import shutil | ||
| import numpy as np | ||
| import pytest | ||
| from forcebalance.parser import parse_inputs | ||
| from forcebalance.forcefield import FF | ||
| from forcebalance.objective import Objective | ||
| from forcebalance.optimizer import Optimizer | ||
| from forcebalance.liquid import _mbar_weights | ||
| from .__init__ import ForceBalanceTestCase, check_for_openmm | ||
|
|
||
| FIXTURE_DIR = os.path.join(os.path.dirname(__file__), 'files', 'test_liquid') | ||
|
|
||
| # U_kln.npy — shape (6, 6, 801), float64 | ||
| # Reduced potential energy matrix U_kln[k, m, n] = (E_k[n] + P_m * V_k[n] * pvkj) * beta_m | ||
| # where k = source simulation index, m = evaluation state index, n = snapshot index. | ||
| # Built from the six npt_result.p pickles in files/test_liquid/single.tmp/Liquid/iter_0000/ | ||
| # (water at 249.15 K/1 atm, 273.15 K/1 atm, 298.15 K/1 atm, 373.15 K/1 atm, | ||
| # 298.15 K/20 bar, 298.15 K/2000 bar; 801 snapshots each). | ||
| # Regenerate with: conda run -n <env> python tools/mbar_mre.py --save-ref | ||
| U_KLN_PATH = os.path.join(FIXTURE_DIR, 'U_kln.npy') | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From here downwards: added tests comparing direct pymbar execution. The script referenced here is attached, not sure if I should add it to the repo or if it's clutter. |
||
|
|
||
| # N_k.npy — shape (6,), int64, all entries = 801 | ||
| # Number of uncorrelated snapshots per simulation state; matches the first axis of U_kln. | ||
| N_K_PATH = os.path.join(FIXTURE_DIR, 'N_k.npy') | ||
|
|
||
| # mbar_weights_ref.npy — shape (4806, 6), float64 | ||
| # MBAR weight matrix W[n, m] produced by pymbar 3.0.5 on U_kln above. | ||
| # Used as the reference for cross-version agreement checks (atol=1e-4). | ||
| # Regenerate with: conda run -n <env> python tools/mbar_mre.py --save-ref | ||
| REF_PATH = os.path.join(FIXTURE_DIR, 'mbar_weights_ref.npy') | ||
|
|
||
|
|
||
| @pytest.fixture(scope='module') | ||
| def mbar_weights(): | ||
| if not os.path.exists(U_KLN_PATH) or not os.path.exists(N_K_PATH): | ||
| pytest.skip(f"MBAR fixtures not found in {FIXTURE_DIR}; run tools/mbar_mre.py --save-ref") | ||
| return _mbar_weights(np.load(U_KLN_PATH), np.load(N_K_PATH)) | ||
|
|
||
|
|
||
| def test_mbar_weights_normalized(mbar_weights): | ||
| """MBAR weight matrix columns must each sum to 1 (pymbar v3 and v4).""" | ||
| col_sums = mbar_weights.sum(axis=0) | ||
| assert np.allclose(col_sums, 1.0, atol=1e-6), ( | ||
| f"MBAR weight columns do not sum to 1: {col_sums}" | ||
| ) | ||
|
|
||
|
|
||
| def test_mbar_weights_match_reference(mbar_weights): | ||
| """MBAR weights must agree with the pymbar-v3 reference within 1e-4.""" | ||
| if not os.path.exists(REF_PATH): | ||
| pytest.skip(f"Reference weights not found at {REF_PATH}; run tools/mbar_mre.py --save-ref") | ||
| W_ref = np.load(REF_PATH) | ||
| assert mbar_weights.shape == W_ref.shape, ( | ||
| f"Weight matrix shape mismatch: got {mbar_weights.shape}, expected {W_ref.shape}" | ||
| ) | ||
| np.testing.assert_allclose(mbar_weights, W_ref, atol=1e-7, rtol=1e-2, | ||
| err_msg="MBAR weights differ from v3 reference beyond rtol=1e-2") | ||
|
|
||
| class TestWaterTutorial(ForceBalanceTestCase): | ||
| def setup_method(self, method): | ||
| if not check_for_openmm(): pytest.skip("No OpenMM modules found.") | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v3: used the adaptive solver by default. (https://pymbar.readthedocs.io/en/3.1.1/mbar.html#pymbar.MBAR)
v4: 'robust' uses adaptive, followed by L-BFGS (https://deepwiki.com/choderalab/pymbar/2.1-mbar-implementation) .
I'm not sure why, but plain swapping this out for the plain
adaptiveprotocol results in tests failing, so maybe the fallback to L-BFGS is necessary. @mattwthompson I saw you managed to swap it out foradaptivein your PR, did you notice any differences?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I don't recall, I was probably trying each option until one worked without really understanding the underlying behavior