Enable user defn boresch restraints in ABFE Protocol#2019
Conversation
…/openfe into abfe_user_defn_restraints
IAlibay
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2019 +/- ##
==========================================
- Coverage 94.95% 90.61% -4.34%
==========================================
Files 217 217
Lines 20758 20808 +50
==========================================
- Hits 19710 18856 -854
- Misses 1048 1952 +904
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
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.
| """ | ||
| 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.
| guest_rdmol=guest_rdmol, | ||
| guest_idxs=guest_atom_ids, | ||
| host_idxs=host_atom_ids, | ||
| guest_restraint_atoms_idxs=list(settings.guest_restraint_ids) |
There was a problem hiding this comment.
If one of these is defined and the other one is None, this will crash out here, but i wonder if it would be helpful to fail earlier.
There was a problem hiding this comment.
Good call, we can add a validitor for that.
|
No API break detected ✅ |
2 participants
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