Modify residue names of ligands and cofactors in the SepTop and ABFE protocols - #2118
Modify residue names of ligands and cofactors in the SepTop and ABFE protocols#2118hannahbaumann wants to merge 18 commits into
Conversation
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
… reused by protocols
…eEnergy/openfe into change_names_abfe_septop
|
pre-commit.ci autofix |
hannahbaumann
changed the title
| alchem_comps["stateA"][0], | ||
| alchem_comps["stateB"][0], | ||
| ) | ||
| smc_off_B = {smc_B: smc_B.to_openff()} |
There was a problem hiding this comment.
I decided to change this since this was overwriting the original off molecule which let to the residue info to got lost, so I thought it would be better to just update the position instead of creating a new molecule.
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2118 +/- ##
==========================================
- Coverage 95.14% 90.50% -4.64%
==========================================
Files 213 213
Lines 20916 20970 +54
==========================================
- Hits 19900 18979 -921
- Misses 1016 1991 +975
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:
|
| assert name is not None | ||
| names.add(name) | ||
| alchem_resnames = sorted(names) | ||
| assigned = assign_offmol_residue_metadata( |
There was a problem hiding this comment.
This is really clean, I like it.
|
No API break detected ✅ Griffe output |
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Just a few small things, otherwise lgtm so I'm approving early.
| @@ -890,11 +901,10 @@ class SepTopSolventSetupUnit(SepTopSolventMixin, BaseSepTopSetupUnit): | |||
| def _update_positions( | |||
There was a problem hiding this comment.
If I understand correctly, this now only gets you the offset but doesn't update positions?
If so, can we rename the method name to match what it does?
| _LIGAND_RESNAME, _LIGAND_STEM = "LIG", "LG" | ||
| _COFACTOR_RESNAME, _COFACTOR_STEM = "COF", "CF" |
There was a problem hiding this comment.
It doesn't look like you're using these variables anywhere - generally it's better to not have globally defined variables hanging about outside of a method unless you want to give some kind of user control.
Can you just let the strings be defined in the method itself? If you want the users to control them, it might be better to have it all be defined as kwargs.
| assert all("residue_name" not in a.metadata for a in mol.atoms) | ||
|
|
||
|
|
||
| def _build(benzene_modifications, specs): |
There was a problem hiding this comment.
Can you maybe move all of this to a pytest class so it's clearer that this is a method specifically for the one test? (ideally also renaming _build to something else, but that's not as necessary once it's in a class)
2 participants
This implements now a helper function to assign residue names and numbers, making sure that any user provided name and number stays and doesn't conflict with other assigned names or residue numbers. The ligands and cofactors now get assigned mostly the same name (LIG for the ligands and COF for the cofactors). They are then distiguishable only by the residue number. As we had discussed in the previous PR, that might be more intuitive for people.
LLM / AI generated code disclosure
LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: yes / no
If yes, please provide details here:
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