SepTop Protocol #1057
SepTop Protocol #1057
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1057 +/- ##
==========================================
- Coverage 95.54% 92.35% -3.20%
==========================================
Files 163 172 +9
Lines 12593 14454 +1861
==========================================
+ Hits 12032 13349 +1317
- Misses 561 1105 +544
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:
|
… into start_septop
… into start_septop
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Docs part of the review (will self-merge the proposed changes since they are typos).
|
|
||
| Orientational, or Boresch-style, restraints are automaticallly (unless manually specified) applied between three protein and three ligand atoms using one bond, | ||
| two angle, and three dihedral restraints. Reference atoms are picked based on different criteria, such as the root mean squared | ||
| fluctuation of the atoms in a short MD simulation, the secondary structure of the protein, and the distance between atoms, based on heuristics from Baumann et al. [2]_. |
There was a problem hiding this comment.
TODO (issued raised): update the docs here (and in ABFEs) to reflect the new picking, i.e. it's getting closer to a mix between Hannah's appproach and MDRestraintsGenerator.
| Molecular interactions are modified along an alchemical path using a discrete set of lambda windows. | ||
| For the transformation of ligand A to ligand B in the binding site, the following steps are carried out, starting with ligand A being fully interacting in the binding site while ligand B is decoupled. | ||
|
|
||
| 1. Insert the non-interacting dummy ligand B into the binding site and restrain it using orientational restraints. The contribution of the restraints is calculated analytically. |
There was a problem hiding this comment.
Separate issue raised: the description of the lambda schedule is non symmetric, i.e. number 5 describes transferring the ligand A to solvent, but none of these refer to transferring ligand B from the solvent (I think it's what is meant by "insert the non-interacting dummy", but the language probably needs to be improved).
|
|
||
| The :class:`.ProtocolDAG` of the :class:`SepTopProtocol <.SepTopProtocol>` contains :class:`.ProtocolUnit`\ s from both the complex and solvent transformations. | ||
| This means that both legs of the thermodynamic cycle are constructed and run sequentially in the same :class:`.ProtocolDAG`. This is different from the :class:`.RelativeHybridTopologyProtocol` where the :class:`.ProtocolDAG` only runs a single leg of a thermodynamic cycle. | ||
| If multiple ``protocol_repeats`` are run (default: ``protocol_repeats=3``), the :class:`.ProtocolDAG` contains multiple :class:`.ProtocolUnit`\ s of both complex and solvent transformations. |
There was a problem hiding this comment.
Unclear from this text if that means that the same restraint will be used for all repeats.
|
|
||
| # Create parent directory if it doesn't exist | ||
| filename_basedir = filename.parent | ||
| if not filename_basedir.exists(): |
| from .geometry import BoreschRestraintGeometry, find_boresch_restraint | ||
| from .geometry import ( | ||
| BoreschRestraintGeometry, | ||
| find_boresch_restraint, |
There was a problem hiding this comment.
Not sure why these are necessary, but we'll let isort deal with it.
| assert f.read() == "<data>" | ||
|
|
||
|
|
||
| def test_serialize_gz(tmpdir): |
There was a problem hiding this comment.
A lot of this is duplicated for gz and bz2, they can be parameterized.
| na_A, | ||
| na_B, | ||
| nonbonded, |
There was a problem hiding this comment.
Passing the tags this way is really confusing. Given it's used in only one test, I would suggest moving this to a test class and then specifically checking for the key strings in the test here.
| timestep=to_openmm(integrator_settings.timestep), | ||
| collision_rate=to_openmm(integrator_settings.langevin_collision_rate), | ||
| n_steps=steps_per_iteration, | ||
| reassign_velocities=integrator_settings.reassign_velocities, |
There was a problem hiding this comment.
Needs guarding from virtual sites.
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Review comments for equil_septop_method
| return working_platforms | ||
|
|
||
|
|
||
| def compute_energy( |
There was a problem hiding this comment.
This is best being somewhere else than in conftest.
| This Protocol is based on, and leverages components originating from | ||
| the SepTop implementation from the Mobleylab | ||
| (https://github.com/MobleyLab/SeparatedTopologies) as well as | ||
| femto (https://github.com/Psivant/femto). |
There was a problem hiding this comment.
We should update the text to reflect "inspired by" rather than "leverages components from".
| Current limitations | ||
| ------------------- | ||
| * Only small molecules are allowed to act as alchemical molecules. |
| Chem.SanitizeMol(rdmol_A) | ||
| Chem.SanitizeMol(rdmol_B) |
There was a problem hiding this comment.
Opened issue #1526 - we should document at the very least.
| from openff.toolkit.topology import Molecule as OFFMolecule | ||
| from openff.units import unit | ||
| from openff.units.openmm import ensure_quantity, from_openmm, to_openmm | ||
| from openmm import app |
There was a problem hiding this comment.
There's a few instances where you use openmm.app and others where app is used directly. That should be cleaned up.
| smc_comps_B = smc_off_B | smc_off_both | ||
| smc_comps_AB = smc_off_A | smc_off_B | smc_off_both | ||
|
|
||
| return smc_comps_A, smc_comps_B, smc_comps_AB, smc_off_B |
There was a problem hiding this comment.
Yeah, although if it works for now it's not a high priority.
|
No API break detected ✅ |
5 participants
Checklist
newsentryDevelopers certificate of origin