Enable user defn boresch restraints in ABFE Protocol - #2019
Conversation
…/openfe into abfe_user_defn_restraints
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2019 +/- ##
==========================================
- Coverage 95.15% 90.57% -4.58%
==========================================
Files 213 213
Lines 20950 21104 +154
==========================================
- Hits 19934 19114 -820
- Misses 1016 1990 +974
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
|
pre-commit.ci autofix |
hannahbaumann
left a comment
There was a problem hiding this comment.
Thanks @IAlibay , this looks great, just added a few comments.
| host_restraint_ids: tuple[int, int, int] | None = None | ||
| """ | ||
| The indices of the host component atoms to restrain. | ||
| The entries define the H0, H1, and H2 atoms in order. |
There was a problem hiding this comment.
Would it be helpful to add how the restraints are defined, since people use different definitions of H0 vs H2 (meaning, is the bond between H0 and G0 or between H2 and G0?)
There was a problem hiding this comment.
I'll double check but I think the docstring gets inherited from
Which has the whole description of the restraints including my attempt at ASCII art.
There was a problem hiding this comment.
Looks like the class docstring wasn't getting inherited, so I've added it.
| """ | ||
| guest_restraint_ids: tuple[int, int, int] | None = None | ||
| """ | ||
| The indices of the guest component atoms to restraint. |
There was a problem hiding this comment.
This is based on the only ligand indices (from the sdf), correct? Maybe worth adding that this are not the indices the ligand would have in the complex (probably obvious since the complex is not created yet, but maybe just to be extra clear).
|
|
||
| errmsg = "``guest_atoms`` and ``host_atoms`` cannot have negative indices." | ||
| with pytest.raises(ValueError, match=errmsg): | ||
| setattr(setting, parameter, [1, 2, -3]) |
There was a problem hiding this comment.
What happens if you pass in 4 integers?
There was a problem hiding this comment.
It'll fail validation, I initially tried that test and was pleasantly surprised that pydantic was that good.
There was a problem hiding this comment.
Added a test just in case.
There was a problem hiding this comment.
@hannahbaumann something I've been thinking about is whether or not SepTop should be using two of these settings (one for each restraint) or if it always makes sense for the restraint to have the exact same settings? (i.e. I think if the settings are different, the restraint might not cancel out, but I'm not sure).
| return v | ||
|
|
||
|
|
||
| class ABFEBoreschRestraintSettings(BoreschRestraintSettings): |
There was a problem hiding this comment.
I'd like to avoid having to proliferate more settings classes for SepTop - TBD with @hannahbaumann, how easily can we align things?
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
…/openfe into abfe_user_defn_restraints
|
|
||
| .. module:: openfe.protocols.openmm_afe.equil_afe_settings | ||
|
|
||
|
|
There was a problem hiding this comment.
I just noticed looking at the API docs that this page openmm_protocol_settings.rst does not have a link to the ABFE protocol settings. Maybe we could add that in this PR?
hannahbaumann
left a comment
There was a problem hiding this comment.
Thanks @IAlibay lgtm! Just the one comment on adding the link to the ABFE ProtocolSettings in the OpenMM settings reference, but approving early!
* User supplied restraints SepTop protocol * Apply suggestion from @hannahbaumann * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update test data * Update test data * Address review comments * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
hannahbaumann
left a comment
There was a problem hiding this comment.
Thanks @IAlibay , lgtm!
|
🚨 API breaking changes detected! 🚨 Griffe output |
Fixes #2002
Checklist
newsentry, or the changes are not user-facing.pre-commit.ci autofix.Manual Tests: these are slow so don't need to be run every commit, only before merging and when relevant changes are made (generally at reviewer-discretion).
Developers certificate of origin