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 <IAlibay@users.noreply.github.com>

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

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