From 8ebfea708556aaa95722d312a4e18cd91a7a2d77 Mon Sep 17 00:00:00 2001 From: Josh Horton Date: Thu, 21 Nov 2024 15:58:52 +0000 Subject: [PATCH] Change optional filter ordering (#64) * add failing test with broken ring mappings * change filtering order * add details on filter order * fix typo --------- Co-authored-by: Irfan Alibay --- src/kartograf/atom_mapper.py | 10 ++++++--- src/kartograf/tests/test_atom_mapper.py | 29 ++++++++++++++++++++++++- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/kartograf/atom_mapper.py b/src/kartograf/atom_mapper.py index 24384d0..980f7e2 100644 --- a/src/kartograf/atom_mapper.py +++ b/src/kartograf/atom_mapper.py @@ -99,13 +99,17 @@ def __init__( with this optional parameter you can further filter the distance based mappings with your own custom filters, provided as iterables. as default we suggest to avoid ring size/breaking changes and only - allow whole rings to be mapped + allow whole rings to be mapped. These filters will be applied before any defaults _mapping_algorithm : str, optional mapping_algorithm.linear_sum_assignment - this allows to swap the optimization algorithm. Not recommended. allow_partial_fused_rings: bool If we should allow partially mapped fused rings (True) or not (False). Default True. + Notes + ----- + The ``additional_mapping_filter_functions`` will be applied before any default filters, to change this + turn off all defaults and pass the full list of filters in the order they should be applied. """ self.atom_max_distance = atom_max_distance self.atom_map_hydrogens = atom_map_hydrogens @@ -114,6 +118,8 @@ def __init__( self.allow_partial_fused_rings = allow_partial_fused_rings self._filter_funcs = [] + if additional_mapping_filter_functions is not None: + self._filter_funcs.extend(additional_mapping_filter_functions) if map_hydrogens_on_hydrogens_only: self._filter_funcs.append(filter_atoms_h_only_h_mapped) if self._map_exact_ring_matches_only: @@ -126,8 +132,6 @@ def __init__( ) if not self.allow_partial_fused_rings: self._filter_funcs.append(filter_fused_ring_changes) - if additional_mapping_filter_functions is not None: - self._filter_funcs.extend(additional_mapping_filter_functions) if _mapping_algorithm == mapping_algorithm.linear_sum_assignment: self._map_hydrogens_on_hydrogens_only = True diff --git a/src/kartograf/tests/test_atom_mapper.py b/src/kartograf/tests/test_atom_mapper.py index c444dbe..a30b0d3 100644 --- a/src/kartograf/tests/test_atom_mapper.py +++ b/src/kartograf/tests/test_atom_mapper.py @@ -5,7 +5,12 @@ from kartograf import KartografAtomMapper from kartograf.atom_mapper import (filter_atoms_h_only_h_mapped, filter_whole_rings_only) -from kartograf.filters.ring_changes import filter_hybridization_rings +from kartograf.filters.element_change import filter_hybridization_changes +from kartograf.filters.ring_changes import ( + filter_whole_rings_only, + filter_hybridization_rings, +) + from .conftest import ( naphtalene_benzene_molecules, @@ -291,6 +296,28 @@ def test_partial_fused_rings(fused_ring_mols, allow_partial_fused_rings, expecte check_mapping_vs_expected(mapping=geom_mapping, expected_mapping=expected_mapping) +def test_hybridization_and_ring_breaks(shp2_hybridization_ligands): + """ + Make sure rings are not broken when map_exact_ring_matches_only=True and a custom filter is used. + """ + mapper = KartografAtomMapper( + map_exact_ring_matches_only=True, + additional_mapping_filter_functions=[filter_hybridization_changes] + ) + mapping = next(mapper.suggest_mappings( + shp2_hybridization_ligands[0], + shp2_hybridization_ligands[1] + )) + # check the whole rings are mapped + filtered_mapping = filter_whole_rings_only( + mapping.componentA.to_rdkit(), + mapping.componentB.to_rdkit(), + mapping.componentA_to_componentB + ) + # make sure there was no change in the mapping + assert filtered_mapping == mapping.componentA_to_componentB + + def test_ring_hybridization_with_non_ring_atoms(shp2_hybridization_ligands): """ Make sure this filter does not fail on non-ring atoms see