Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1561 +/- ##
==========================================
- Coverage 94.79% 91.05% -3.74%
==========================================
Files 205 205
Lines 17957 18342 +385
==========================================
- Hits 17022 16702 -320
- Misses 935 1640 +705
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
| protocol_settings.forcefield_settings.forcefields = [ | ||
| "amber/ff14SB.xml", | ||
| "amber/tip3p_standard.xml", | ||
| "amber/tip3p_HFE_multivalent.xml", | ||
| "amber/lipid17_merged.xml", | ||
| "amber/phosaa10.xml", | ||
| ] |
There was a problem hiding this comment.
Should this be changing all of the force fields? What if the user has requested tip4p and ff19sb? This silently replaces the force field which is not good, we could check if its in the list and if not add it or maybe we should update the default list to include the lipid force field I don't think there is any harm in doing that.
| protocol_settings.forcefield_settings.forcefields = [ | |
| "amber/ff14SB.xml", | |
| "amber/tip3p_standard.xml", | |
| "amber/tip3p_HFE_multivalent.xml", | |
| "amber/lipid17_merged.xml", | |
| "amber/phosaa10.xml", | |
| ] | |
| if "amber/lipid17_merged.xml" not in protocol_settings.forcefield_settings.forcefields: | |
| protocol_settings.forcefield_settings.forcefields.append("amber/lipid17_merged.xml") |
There was a problem hiding this comment.
maybe we should update the default list to include the lipid force field I don't think there is any harm in doing that.
I think this is more sensible. It would be a problem if someone wanted another lipid FF and then got lipid 17 instead.
There was a problem hiding this comment.
Opened this gufe PR to fix it: OpenFreeEnergy/gufe#767
| # roundtrip box vectors to remove vec3 issues | ||
| box = to_openmm(from_openmm(stateA_system.getDefaultPeriodicBoxVectors())) | ||
| stateA_topology.setPeriodicBoxVectors(box) |
There was a problem hiding this comment.
Should this not be done in system creation when setting the box vectors from the membrane protein component?
| elif isinstance(solvent_comp, SolvatedPDBComponent): | ||
| # Set the periodic box vectors | ||
| system_modeller.topology.setPeriodicBoxVectors(to_openmm(solvent_comp.box_vectors)) |
There was a problem hiding this comment.
Can we do the vec3 round trip stuff here to avoid it being done in protocols?
| solvent = SolventComponent(ion_concentration=0 * offunit.molar) | ||
| num_all_not_water = 16080 | ||
| num_complex_atoms = 39390 | ||
| # No ions | ||
| num_ligand_atoms = 36 |
There was a problem hiding this comment.
Should we be checking that no solvent is added to the system as well?
| # Forcefields should include the lipid forcefields | ||
| ff = settings.forcefield_settings.forcefields | ||
| assert "amber/lipid17_merged.xml" in ff |
There was a problem hiding this comment.
Also include a test to ensure that custom input settings using a different set of force fields are not changed.
|
|
||
| # Check we have the right number of atoms in the PDB | ||
| pdb = mdt.load_pdb("hybrid_system.pdb") | ||
| assert pdb.n_atoms == 4690 |
There was a problem hiding this comment.
should we be checking no extra solvent is added here, by changing the selection to all and making sure the number of atoms don't change?
|
|
||
| # Forcefields should include the lipid forcefields | ||
| ff = settings.forcefield_settings.forcefields | ||
| assert "amber/lipid17_merged.xml" in ff |
There was a problem hiding this comment.
Lets also add a test here to make sure we don't break the users input settings if they have already changed the default force fields.
| box_modeller = model.topology.getPeriodicBoxVectors() | ||
| box_protein = a2a_protein_membrane_component.box_vectors | ||
|
|
||
| assert np.allclose(box_modeller, to_openmm(box_protein), atol=1e-6) |
There was a problem hiding this comment.
check no extra water or ions have been added?
jthorton
left a comment
jthorton
left a comment
There was a problem hiding this comment.
Looking super close, spotted one bug with adaptive settings which should be fixed, it would also be nice to explicitly test that we are not adding waters to membrane systems before considering merging the others are optional.
Co-authored-by: Josh Horton <joshua.horton@openforcefield.org>
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
|
No API break detected ✅ |
4 participants
[Note from @atravitz: we are making an experimental release from this branch, and so this branch should not be merged into main just yet]
This PR addresses the following issues:
Checklist
newsentryDevelopers certificate of origin