From 7414d951ede5e3f82df1c6484e4f14709ad85e8a Mon Sep 17 00:00:00 2001 From: richard gowers Date: Thu, 8 Feb 2024 12:56:17 +0000 Subject: [PATCH 1/5] LAM: _annotations doesn't need to be frozen since we're only exposing a copy of the annotations it's safe enough --- gufe/mapping/ligandatommapping.py | 1 - 1 file changed, 1 deletion(-) diff --git a/gufe/mapping/ligandatommapping.py b/gufe/mapping/ligandatommapping.py index ca240cb2..b0650132 100644 --- a/gufe/mapping/ligandatommapping.py +++ b/gufe/mapping/ligandatommapping.py @@ -45,7 +45,6 @@ def __init__( self._compA_to_compB = componentA_to_componentB if annotations is None: - # TODO: this should be a frozen dict annotations = {} self._annotations = annotations From 37b4cb81a4797fd754e8535d66e22ca0b0780b70 Mon Sep 17 00:00:00 2001 From: richard gowers Date: Thu, 8 Feb 2024 13:24:11 +0000 Subject: [PATCH 2/5] tests: update LigandAtomMapping tests use Chem.AddHs() on fixtures to make test cases more realistic this changes the hash of the resulting test as the hash of the SmallMoleculeComponent within has changed --- gufe/tests/test_ligandatommapping.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/gufe/tests/test_ligandatommapping.py b/gufe/tests/test_ligandatommapping.py index c4fb01da..30615d4c 100644 --- a/gufe/tests/test_ligandatommapping.py +++ b/gufe/tests/test_ligandatommapping.py @@ -13,11 +13,11 @@ from .test_tokenization import GufeTokenizableTestsMixin -def mol_from_smiles(smiles: str) -> Chem.Mol: - m = Chem.MolFromSmiles(smiles) +def mol_from_smiles(smiles: str) -> gufe.SmallMoleculeComponent: + m = Chem.AddHs(Chem.MolFromSmiles(smiles)) m.Compute2DCoords() - return m + return gufe.SmallMoleculeComponent(m) @pytest.fixture(scope='session') @@ -28,8 +28,8 @@ def simple_mapping(): C C """ - molA = gufe.SmallMoleculeComponent(mol_from_smiles('CCO')) - molB = gufe.SmallMoleculeComponent(mol_from_smiles('CC')) + molA = mol_from_smiles('CCO') + molB = mol_from_smiles('CC') m = LigandAtomMapping(molA, molB, componentA_to_componentB={0: 0, 1: 1}) @@ -44,8 +44,8 @@ def other_mapping(): C C """ - molA = SmallMoleculeComponent(mol_from_smiles('CCO')) - molB = SmallMoleculeComponent(mol_from_smiles('CC')) + molA = mol_from_smiles('CCO') + molB = mol_from_smiles('CC') m = LigandAtomMapping(molA, molB, componentA_to_componentB={0: 0, 2: 1}) @@ -239,7 +239,7 @@ def test_with_fancy_annotations(simple_mapping): class TestLigandAtomMapping(GufeTokenizableTestsMixin): cls = LigandAtomMapping repr = "LigandAtomMapping(componentA=SmallMoleculeComponent(name=), componentB=SmallMoleculeComponent(name=), componentA_to_componentB={0: 0, 1: 1}, annotations={'foo': 'bar'})" - key = "LigandAtomMapping-c95a2c15fe21f446cf731338427137ae" + key = "LigandAtomMapping-c333723fbbee702c641cb9dca9beae49" @pytest.fixture def instance(self, annotated_simple_mapping): From 377cc00820b7886442ac07771c1828bc2d96367c Mon Sep 17 00:00:00 2001 From: richard gowers Date: Thu, 8 Feb 2024 13:25:24 +0000 Subject: [PATCH 3/5] adds validation for LigandAtomMapping catches some cases where bad indices are provided fixes #170 --- gufe/mapping/ligandatommapping.py | 16 ++++++++++++- gufe/tests/test_ligandatommapping.py | 36 ++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/gufe/mapping/ligandatommapping.py b/gufe/mapping/ligandatommapping.py index b0650132..c77f0859 100644 --- a/gufe/mapping/ligandatommapping.py +++ b/gufe/mapping/ligandatommapping.py @@ -34,7 +34,9 @@ def __init__( componentA_to_componentB : dict[int, int] correspondence of indices of atoms between the two ligands; the keys are indices in componentA and the values are indices in - componentB + componentB. + These are checked that they are within the possible indices of the + respective components. annotations : dict[str, Any] Mapping of annotation identifier to annotation data. Annotations may contain arbitrary JSON-serializable data. Annotation identifiers @@ -42,6 +44,18 @@ def __init__( OpenFE. ``score`` is a reserved annotation identifier. """ super().__init__(componentA, componentB) + + # validate compA_to_compB + nA = self.componentA.to_rdkit().GetNumAtoms() + nB = self.componentB.to_rdkit().GetNumAtoms() + for i, j in componentA_to_componentB.items(): + if not (0 <= i < nA): + raise ValueError(f"Got invalid index for ComponentA ({i}); " + f"must be 0 <= n <= {nA}") + if not (0 <= j < nB): + raise ValueError(f"Got invalid index for ComponentB ({i}); " + f"must be 0 <= n <= {nB}") + self._compA_to_compB = componentA_to_componentB if annotations is None: diff --git a/gufe/tests/test_ligandatommapping.py b/gufe/tests/test_ligandatommapping.py index 30615d4c..edd7b75b 100644 --- a/gufe/tests/test_ligandatommapping.py +++ b/gufe/tests/test_ligandatommapping.py @@ -236,6 +236,42 @@ def test_with_fancy_annotations(simple_mapping): assert m == m2 +class TestLigandAtomMappingBoundsChecks: + @pytest.fixture + def molA(self): + # 9 atoms + return mol_from_smiles('CCO') + + @pytest.fixture + def molB(self): + # 11 atoms + return mol_from_smiles('CCC') + + def test_too_large_A(self, molA, molB): + with pytest.raises(ValueError, match="invalid index for ComponentA"): + LigandAtomMapping(componentA=molA, + componentB=molB, + componentA_to_componentB={9: 5}) + + def test_too_small_A(self, molA, molB): + with pytest.raises(ValueError, match="invalid index for ComponentA"): + LigandAtomMapping(componentA=molA, + componentB=molB, + componentA_to_componentB={-2: 5}) + + def test_too_large_B(self, molA, molB): + with pytest.raises(ValueError, match="invalid index for ComponentB"): + LigandAtomMapping(componentA=molA, + componentB=molB, + componentA_to_componentB={5: 11}) + + def test_too_small_B(self, molA, molB): + with pytest.raises(ValueError, match="invalid index for ComponentB"): + LigandAtomMapping(componentA=molA, + componentB=molB, + componentA_to_componentB={5: -1}) + + class TestLigandAtomMapping(GufeTokenizableTestsMixin): cls = LigandAtomMapping repr = "LigandAtomMapping(componentA=SmallMoleculeComponent(name=), componentB=SmallMoleculeComponent(name=), componentA_to_componentB={0: 0, 1: 1}, annotations={'foo': 'bar'})" From 7d08479d13410610bb7bff372a78af842ee806be Mon Sep 17 00:00:00 2001 From: richard gowers Date: Thu, 8 Feb 2024 13:25:42 +0000 Subject: [PATCH 4/5] some doc fixes and typing hints for LigandAtomMapping --- gufe/mapping/ligandatommapping.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/gufe/mapping/ligandatommapping.py b/gufe/mapping/ligandatommapping.py index c77f0859..75c81969 100644 --- a/gufe/mapping/ligandatommapping.py +++ b/gufe/mapping/ligandatommapping.py @@ -1,5 +1,7 @@ # This code is part of gufe and is licensed under the MIT license. # For details, see https://github.com/OpenFreeEnergy/gufe +from __future__ import annotations + import json from typing import Any, Optional import numpy as np @@ -18,6 +20,8 @@ class LigandAtomMapping(AtomMapping): """ componentA: SmallMoleculeComponent componentB: SmallMoleculeComponent + _annotations: dict[str, Any] + _compA_to_compB: dict[int, int] def __init__( self, @@ -87,6 +91,7 @@ def componentB_unique(self): @property def annotations(self): + """Any extra metadata, for example the score of a mapping""" # return a copy (including copy of nested) return json.loads(json.dumps(self._annotations)) @@ -165,8 +170,8 @@ def draw_to_file(self, fname: str, d2d=None): f.write(draw_mapping(self._compA_to_compB, self.componentA.to_rdkit(), self.componentB.to_rdkit(), d2d)) - def with_annotations(self, annotations: dict[str, Any]): - """Create an new mapping based on this one with extra annotations. + def with_annotations(self, annotations: dict[str, Any]) -> LigandAtomMapping: + """Create a new mapping based on this one with extra annotations. Parameters ---------- From 6c6318d090b8a165e362ad6dc3c616b0aebf2d1c Mon Sep 17 00:00:00 2001 From: Richard Gowers Date: Fri, 9 Feb 2024 09:58:33 +0000 Subject: [PATCH 5/5] Update ligandatommapping.py --- gufe/mapping/ligandatommapping.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gufe/mapping/ligandatommapping.py b/gufe/mapping/ligandatommapping.py index 75c81969..e7acb6df 100644 --- a/gufe/mapping/ligandatommapping.py +++ b/gufe/mapping/ligandatommapping.py @@ -55,10 +55,10 @@ def __init__( for i, j in componentA_to_componentB.items(): if not (0 <= i < nA): raise ValueError(f"Got invalid index for ComponentA ({i}); " - f"must be 0 <= n <= {nA}") + f"must be 0 <= n < {nA}") if not (0 <= j < nB): raise ValueError(f"Got invalid index for ComponentB ({i}); " - f"must be 0 <= n <= {nB}") + f"must be 0 <= n < {nB}") self._compA_to_compB = componentA_to_componentB