-
Notifications
You must be signed in to change notification settings - Fork 52
Fix residue naming #2042
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
hannahbaumann
wants to merge
45
commits into
main
Choose a base branch
from
fix_residue_naming
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Fix residue naming #2042
Changes from all commits
Commits
Show all changes
45 commits
Select commit
Hold shift + click to select a range
e7882f0
Initial change of lig and cofactor resname
hannahbaumann cb4ece2
Merge branch 'main' into fix_residue_naming
hannahbaumann a32cf02
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 369f9d5
Fix tests
hannahbaumann 69a6a08
Add news entry
hannahbaumann 2609bc4
Add more tests
hannahbaumann 0844937
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 91fde04
Merge branch 'main' into fix_residue_naming
hannahbaumann d7a36cd
Merge branch 'main' into fix_residue_naming
IAlibay 11b9c2c
Update src/openfe/tests/protocols/openmm_rfe/test_hybrid_top_protocol.py
hannahbaumann 12b9840
Address review comments
hannahbaumann c68b88e
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] de84279
Update src/openfe/tests/protocols/openmm_rfe/test_hybrid_top_protocol.py
hannahbaumann 8fca835
address review comments
hannahbaumann 9b21e80
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 6ae227d
Make mypy happy
hannahbaumann def4a76
Make mypy happy
hannahbaumann 3bcbca8
Address review comments
hannahbaumann 9deae8a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 6fdcb74
Apply suggestion from @hannahbaumann
hannahbaumann 465045e
Make mypy happy
hannahbaumann be7ed8a
Merge branch 'fix_residue_naming' of https://github.com/OpenFreeEnerg…
hannahbaumann e88e61b
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] de270f0
Small fix
hannahbaumann 13585c1
Merge branch 'fix_residue_naming' of https://github.com/OpenFreeEnerg…
hannahbaumann 22cc7dc
Merge branch 'main' into fix_residue_naming
IAlibay dae148e
Merge branch 'main' into fix_residue_naming
hannahbaumann f7b438c
Merge branch 'main' into fix_residue_naming
hannahbaumann cea0a89
address review comments
hannahbaumann aa8fba2
Merge branch 'fix_residue_naming' of https://github.com/OpenFreeEnerg…
hannahbaumann c63caaa
Fix mypy
hannahbaumann 4729087
update comments
hannahbaumann 9e4b9d2
update comments
hannahbaumann aaa22c5
Address review comments
hannahbaumann 6b31ed5
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 1bc9366
Update news/fix_unk_resnames.rst
hannahbaumann bc2bb0b
Address review comments
hannahbaumann 73d9c93
Address review comments
hannahbaumann e15d56d
small fix
hannahbaumann c818e3e
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 7277c00
Apply suggestion from @hannahbaumann
hannahbaumann 7057e15
Apply suggestion from @IAlibay
hannahbaumann f0ac4f8
fix alchem_resname grab
hannahbaumann 8b07a3d
Merge branch 'fix_residue_naming' of https://github.com/OpenFreeEnerg…
hannahbaumann 4c8a2ea
Merge branch 'main' into fix_residue_naming
hannahbaumann File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| **Added:** | ||
|
|
||
| * <news item> | ||
|
|
||
| **Changed:** | ||
|
|
||
| * Small molecules in RelativeHybridTopologyProtocol topologies (including the output PDB) are now named LIG (alchemical ligand) and COF (cofactors) instead of UNK. If a residue name was already assigned, the assigned one is kept. | ||
|
|
||
| **Deprecated:** | ||
|
|
||
| * <news item> | ||
|
|
||
| **Removed:** | ||
|
|
||
| * <news item> | ||
|
|
||
| **Fixed:** | ||
|
|
||
| * Fixed inflated ligand RMSD in the RelativeHybridTopology protocol's structural analysis for systems containing cofactors; the ligand RMSD is now computed for the alchemical ligand alone rather than conflating it with cofactors that shared the UNK residue name. | ||
|
|
||
| **Security:** | ||
|
|
||
| * <news item> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,11 @@ | |
| from openmmtools import multistate | ||
|
|
||
| import openfe | ||
| from openfe.protocols.openmm_utils.offmolecule_utils import ( | ||
| _get_offmol_resname, | ||
| _set_offmol_metadata, | ||
| _set_offmol_resname, | ||
| ) | ||
| from openfe.protocols.openmm_utils.omm_settings import ( | ||
| BasePartialChargeSettings, | ||
| ) | ||
|
|
@@ -753,6 +758,46 @@ def run( | |
| alchem_comps = self._inputs["alchemical_components"] | ||
| solvent_comp, protein_comp, small_mols = self._get_components(stateA, stateB) | ||
|
|
||
| alchemical = set(alchem_comps["stateA"]) | set(alchem_comps["stateB"]) | ||
|
|
||
| def _unique(stem: str, used: set[str]) -> str: | ||
| for i in range(1, 10): | ||
| candidate = f"{stem}{i}" | ||
| if candidate not in used: | ||
| return candidate | ||
| raise ValueError( | ||
| f"Could not assign a unique residue name with stem {stem!r}; " | ||
| "too many colliding names." | ||
| ) | ||
|
|
||
| # Seed with user-provided resnames so auto-assigned names avoid them. | ||
| used: set[str] = set() | ||
| for offmol in small_mols.values(): | ||
| name = _get_offmol_resname(offmol) | ||
| if name is not None: | ||
| used.add(name) | ||
|
|
||
| lig_name = "LIG" if "LIG" not in used else _unique("LG", used) | ||
|
|
||
| resnum = 1 | ||
| for smc, offmol in small_mols.items(): | ||
| if _get_offmol_resname(offmol) is not None: | ||
| continue | ||
| if smc in alchemical: | ||
| name = lig_name | ||
| else: | ||
| name = "COF" if "COF" not in used else _unique("CO", used) | ||
| _set_offmol_resname(offmol, name) | ||
| _set_offmol_metadata(offmol, "residue_number", resnum) | ||
| resnum += 1 | ||
|
|
||
| names: set[str] = set() | ||
| for comp in alchemical: | ||
| name = _get_offmol_resname(small_mols[comp]) | ||
| assert name is not None | ||
| names.add(name) | ||
| alchem_resnames = sorted(names) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] not something to fix now, but just an FYI that long term we might need to know what name belongs to what end state. |
||
|
|
||
| # Assign partial charges now to avoid any discrepancies later | ||
| self._assign_partial_charges(settings["charge_settings"], small_mols) | ||
|
|
||
|
|
@@ -810,6 +855,7 @@ def run( | |
| "positions": positions_outfile, | ||
| "pdb_structure": self.shared_basepath / settings["output_settings"].output_structure, | ||
| "selection_indices": selection_indices, | ||
| "alchemical_resnames": alchem_resnames, | ||
| } | ||
|
|
||
| if dry: | ||
|
|
@@ -1505,6 +1551,7 @@ def _structural_analysis( | |
| trj_file: pathlib.Path, | ||
| output_directory: pathlib.Path, | ||
| dry: bool, | ||
| ligand_resnames: list[str], | ||
| ) -> dict[str, str | pathlib.Path]: | ||
| """ | ||
| Run structural analysis using ``openfe-analysis``. | ||
|
|
@@ -1520,6 +1567,8 @@ def _structural_analysis( | |
| will be stored. | ||
| dry : bool | ||
| Whether or not we are running a dry run. | ||
| ligand_resnames: list[str] | ||
| The residue names of the ligands. | ||
|
|
||
| Returns | ||
| ------- | ||
|
|
@@ -1535,7 +1584,9 @@ def _structural_analysis( | |
| from openfe_analysis import rmsd | ||
|
|
||
| try: | ||
| data = rmsd.gather_rms_data(pdb_file, trj_file) | ||
| data = rmsd.gather_rms_data( | ||
| pdb_file, trj_file, ligand_selection="resname " + " ".join(ligand_resnames) | ||
| ) | ||
| # TODO: eventually change this to more specific exception types | ||
| except Exception as e: | ||
| return {"structural_analysis_error": str(e)} | ||
|
|
@@ -1573,6 +1624,7 @@ def run( | |
| pdb_file: pathlib.Path, | ||
| trajectory: pathlib.Path, | ||
| checkpoint: pathlib.Path, | ||
| ligand_resnames: list[str], | ||
| dry: bool = False, | ||
| verbose: bool = True, | ||
| scratch_basepath: pathlib.Path | None = None, | ||
|
|
@@ -1588,6 +1640,8 @@ def run( | |
| Path to the MultiStateReporter generated NetCDF file. | ||
| checkpoint : pathlib.Path | ||
| Path to the checkpoint file generated by MultiStateReporter. | ||
| ligand_resnames: list[str] | ||
| The residue names of the ligands. | ||
| dry : bool | ||
| Do a dry run of the calculation, creating all necessary hybrid | ||
| system components (topology, system, sampler, etc...) but without | ||
|
|
@@ -1641,6 +1695,7 @@ def run( | |
| trj_file=trajectory, | ||
| output_directory=self.shared_basepath, | ||
| dry=dry, | ||
| ligand_resnames=ligand_resnames, | ||
|
hannahbaumann marked this conversation as resolved.
|
||
| ) | ||
|
|
||
| # Return relevant things | ||
|
|
@@ -1664,11 +1719,13 @@ def _execute( | |
| selection_indices = setup_results.outputs["selection_indices"] | ||
| trajectory = simulation_results.outputs["nc"] | ||
| checkpoint = simulation_results.outputs["checkpoint"] | ||
| ligand_resnames = setup_results.outputs["alchemical_resnames"] | ||
|
|
||
| outputs = self.run( | ||
| pdb_file=pdb_file, | ||
| trajectory=trajectory, | ||
| checkpoint=checkpoint, | ||
| ligand_resnames=ligand_resnames, | ||
| scratch_basepath=ctx.scratch, | ||
| shared_basepath=ctx.shared, | ||
| ) | ||
|
|
||
|
IAlibay marked this conversation as resolved.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| # This code is part of OpenFE and is licensed under the MIT license. | ||
| # For details, see https://github.com/OpenFreeEnergy/openfe | ||
| import logging | ||
| from typing import Any | ||
|
|
||
| from openff.toolkit import Molecule as OFFMolecule | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def _set_offmol_metadata( | ||
| offmol: OFFMolecule, | ||
| key: Any, | ||
| val: Any | None, | ||
| ) -> None: | ||
| """ | ||
| Set a given metadata entry for a whole Molecule. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| offmol : openff.toolkit.Molecule | ||
| The Molecule to set the metadata for. | ||
| key : Any | ||
| The metadata key. | ||
| val : Any | ||
| The value to set the metadata entry to. | ||
| """ | ||
| if val is None: | ||
| for a in offmol.atoms: | ||
| a.metadata.pop(key, None) | ||
| else: | ||
| for a in offmol.atoms: | ||
| a.metadata[key] = val | ||
|
|
||
|
|
||
| def _get_offmol_metadata(offmol: OFFMolecule, key: Any) -> Any | None: | ||
| """ | ||
| Get an offmol's given metadata entry and make sure it is | ||
| consistent across all atoms in the Molecule. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| offmol : openff.toolkit.Molecule | ||
| Molecule to get the metadata value from. | ||
| key: Any | ||
| The metadata entry key. | ||
|
|
||
| Returns | ||
| ------- | ||
| value : Any | None | ||
| Metadata for the given key in the molecule. ``None`` if the | ||
| Molecule does not have that metadata entry set, or if | ||
| the value is inconsistent across all the atoms. | ||
| """ | ||
| value: Any | None = None | ||
| for a in offmol.atoms: | ||
| if value is None: | ||
| try: | ||
| value = a.metadata[key] | ||
| except KeyError: | ||
| return None | ||
|
|
||
| if value != a.metadata[key]: | ||
| wmsg = f"Inconsistent metadata {key} in OFFMol: {offmol}" | ||
| logger.warning(wmsg) | ||
| return None | ||
|
|
||
| return value | ||
|
|
||
|
|
||
| def _set_offmol_resname( | ||
| offmol: OFFMolecule, | ||
| resname: str | None, | ||
| ) -> None: | ||
| """ | ||
| Helper method to set offmol residue names | ||
|
|
||
| Parameters | ||
| ---------- | ||
| offmol : openff.toolkit.Molecule | ||
| Molecule to assign a residue name to. | ||
| resname : str | None | ||
| Residue name to be set. Set to None to clear it. | ||
|
|
||
| Returns | ||
| ------- | ||
| None | ||
| """ | ||
| _set_offmol_metadata(offmol, "residue_name", resname) | ||
|
|
||
|
|
||
| def _get_offmol_resname(offmol: OFFMolecule) -> str | None: | ||
| """ | ||
| Helper method to get an offmol's residue name and make sure it is | ||
| consistent across all atoms in the Molecule. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| offmol : openff.toolkit.Molecule | ||
| Molecule to get the residue name from. | ||
|
|
||
| Returns | ||
| ------- | ||
| resname : Optional[str] | ||
| Residue name of the molecule. ``None`` if the Molecule | ||
| does not have a residue name, or if the residue name is | ||
| inconsistent across all the atoms. | ||
| """ | ||
| return _get_offmol_metadata(offmol, "residue_name") |
32 changes: 16 additions & 16 deletions
32
src/openfe/tests/data/openmm_rfe/benzene_toluene_hybrid_top/hybrid_topology_atoms.csv
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,17 @@ | ||
| ,serial,name,element,resSeq,resName,chainID,segmentID,formal_charge | ||
| 0,,C1x,C,0,UNK,0,, | ||
| 1,,C2x,C,0,UNK,0,, | ||
| 2,,C3x,C,0,UNK,0,, | ||
| 3,,C4x,C,0,UNK,0,, | ||
| 4,,C5x,C,0,UNK,0,, | ||
| 5,,C6x,C,0,UNK,0,, | ||
| 6,,H1x,H,0,UNK,0,, | ||
| 7,,H2x,H,0,UNK,0,, | ||
| 8,,H3x,H,0,UNK,0,, | ||
| 9,,H4x,H,0,UNK,0,, | ||
| 10,,H5x,H,0,UNK,0,, | ||
| 11,,H6x,H,0,UNK,0,, | ||
| 12,,H1x,H,0,UNK,0,, | ||
| 13,,H2x,H,0,UNK,0,, | ||
| 14,,C1x,C,0,UNK,0,, | ||
| 15,,H3x,H,0,UNK,0,, | ||
| 0,,C1x,C,0,LIG,0,, | ||
| 1,,C2x,C,0,LIG,0,, | ||
| 2,,C3x,C,0,LIG,0,, | ||
| 3,,C4x,C,0,LIG,0,, | ||
| 4,,C5x,C,0,LIG,0,, | ||
| 5,,C6x,C,0,LIG,0,, | ||
| 6,,H1x,H,0,LIG,0,, | ||
| 7,,H2x,H,0,LIG,0,, | ||
| 8,,H3x,H,0,LIG,0,, | ||
| 9,,H4x,H,0,LIG,0,, | ||
| 10,,H5x,H,0,LIG,0,, | ||
| 11,,H6x,H,0,LIG,0,, | ||
| 12,,H1x,H,0,LIG,0,, | ||
| 13,,H2x,H,0,LIG,0,, | ||
| 14,,C1x,C,0,LIG,0,, | ||
| 15,,H3x,H,0,LIG,0,, |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can get this down to one loop and I think it makes it easier to read as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i had separated these for the (probably unlikely case) that e.g. a cofactor was named LG1, if it came after the ligand, the LG1 would not be in used yet and both would carry the same residue name. Or is there a different way around that?