Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1561 +/- ##
==========================================
- Coverage 95.39% 93.23% -2.17%
==========================================
Files 189 189
Lines 16646 16922 +276
==========================================
- Hits 15880 15777 -103
- Misses 766 1145 +379
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…y/openfe into membrane_prototype
…y/openfe into membrane_prototype
…y/openfe into membrane_prototype
…y/openfe into membrane_prototype
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
…y/openfe into membrane_prototype
|
No API break detected ✅ |
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
An initial look through - I can't work out why that one energy test is causing a mismatch, there's no direct reason why it should be happening, it looks like it needs dissecting manually.
openfe/tests/data/a2a/protein.pdb
Outdated
openfe/tests/data/a2a/ligands.sdf
Outdated
| self.settings.integrator_settings.timestep, | ||
| self.settings.complex_integrator_settings.timestep, | ||
| ) | ||
| settings_validation.validate_timestep( |
There was a problem hiding this comment.
| settings_validation.validate_timestep( | |
| settings_validation.validate_timestep( |
| @@ -382,12 +383,28 @@ class IntegratorSettings(SettingsBaseModel): | |||
| """ | |||
| constraint_tolerance: float = 1e-06 | |||
| """Tolerance for the constraint solver. Default 1e-6.""" | |||
| barostat: Literal["MonteCarloBarostat", "MonteCarloMembraneBarostat"] = "MonteCarloBarostat" | |||
There was a problem hiding this comment.
If you haven't already, please raise an issue and add it to the 2.0 milestone.
| """ | ||
| assert sampler.is_periodic | ||
| assert isinstance(sampler, MultiStateSampler) | ||
| assert isinstance(sampler._thermodynamic_states[0].barostat, MonteCarloBarostat) |
There was a problem hiding this comment.
why not do the barostat_by_phase here and avoid re-implementing in the child class?
| def _check_box_vectors(self, system): | ||
| self._test_orthogonal_vectors(system) | ||
|
|
||
| def _verify_sampler(self, sampler, phase: str, settings): |
There was a problem hiding this comment.
See above re: avoiding re-implementing here.
| assert pytest.approx(geom[0][0].phi_C0) == -1.068226 * offunit.radian | ||
|
|
||
|
|
||
| class TestA2AMembraneDryRun: |
There was a problem hiding this comment.
Mark this as slow? - it's running for 526.53s on the CI runners.
| assert pytest.approx(prop_chgs[i]) == offsets[2] | ||
|
|
||
|
|
||
| class TestA2AMembraneDryRun(TestT4LysozymeDryRun): |
| return s | ||
|
|
||
| @pytest.fixture(scope="class") | ||
| def dag(self, protocol, a2a_ligands, a2a_protein_membrane_component): |
There was a problem hiding this comment.
It's not immediately clear to me why the energy dissection is failing - there's nothing that should be causing this unless the ligand indices are being poorly handled.
I'm wondering if one of the forces is accumulating some excess energy somehow :/
Edit: see my next comment - it seems like an issue with the non-Reference platforms.
| assert pytest.approx(prop_chgs[i]) == offsets[2] | ||
|
|
||
|
|
||
| class TestA2AMembraneDryRun(TestT4LysozymeDryRun): |
There was a problem hiding this comment.
Ok, so I managed to debug this a bit more, if I set the global alchemy platform to the Reference one here: https://github.com/choderalab/openmmtools/blob/main/openmmtools/tests/test_alchemy.py#L57 the test passes.
However, if I set it to any other platform it fails - I suspect this is a deeper issue with OpenMM.
Note: tested on OpenMM 8.4.
3 participants
This PR addresses the following issues:
Checklist
newsentryDevelopers certificate of origin