Repository: matchms/matchms
Pull request #524 (2023-09-06 12:02:48)

Remove duplicated filters
Duplicated filters will now not be added twice to the pipeline, instead they are added only once. And the settings of the last added filter are used. This would be expected behaviour for me, since sometimes you might want to use the default filters, but use different settings. This makes it possible to just use the default pipeline and only change the parameters for the filter that has to be changed. 


Patch:
diff --git a/matchms/Fragments.py b/matchms/Fragments.py
index 52787605..41801dae 100644
--- a/matchms/Fragments.py
+++ b/matchms/Fragments.py
@@ -45,8 +45,6 @@ class Fragments:
         assert self._is_sorted(), "mz values are out of order."
 
     def __eq__(self, other):
-        if other is None:
-            return False
         return \
             self.mz.shape == other.mz.shape and \
             np.allclose(self.mz, other.mz) and \
diff --git a/matchms/filtering/SpectrumProcessor.py b/matchms/filtering/SpectrumProcessor.py
index 4c5d3b02..f0a9ed1f 100644
--- a/matchms/filtering/SpectrumProcessor.py
+++ b/matchms/filtering/SpectrumProcessor.py
@@ -1,7 +1,8 @@
 import inspect
+import logging
 from collections import defaultdict
 from functools import partial
-from typing import Callable, Dict, Optional, Tuple, Union
+from typing import Dict, Optional, Tuple, Union
 import numpy as np
 import pandas as pd
 from tqdm import tqdm
@@ -10,6 +11,9 @@ from matchms.filtering.filter_order_and_default_pipelines import (
     ALL_FILTERS, FILTER_FUNCTION_NAMES, PREDEFINED_PIPELINES)
 
 
+logger = logging.getLogger("matchms")
+
+
 class SpectrumProcessor:
     """
     A class to process spectra using a series of filters.
@@ -35,8 +39,7 @@ class SpectrumProcessor:
             for filter_name in PREDEFINED_PIPELINES[predefined_pipeline]:
                 self.add_matchms_filter(filter_name)
 
-    def add_filter(self,
-                   filter_function: Union[Tuple[str, Dict[str, any]], str, Tuple[Callable, Dict[str, any]], Callable]):
+    def add_filter(self, filter_function: Union[Tuple[str, Dict[str, any]], str]):
         """Add a filter to the processing pipeline. Takes both matchms filter names (and parameters)
         as well as custom-made functions.
         """
@@ -60,21 +63,32 @@ class SpectrumProcessor:
         if isinstance(filter_spec, str):
             if filter_spec not in FILTER_FUNCTION_NAMES:
                 raise ValueError(f"Unknown filter type: {filter_spec} Should be known filter name or function.")
-            filter_func = FILTER_FUNCTION_NAMES[filter_spec]
+            new_filter_func = FILTER_FUNCTION_NAMES[filter_spec]
         elif isinstance(filter_spec, (tuple, list)):
             filter_name, filter_args = filter_spec
             if filter_name not in FILTER_FUNCTION_NAMES:
                 raise ValueError(f"Unknown filter type: {filter_name} Should be known filter name or function.")
-            filter_func = partial(FILTER_FUNCTION_NAMES[filter_name], **filter_args)
-            filter_func.__name__ = FILTER_FUNCTION_NAMES[filter_name].__name__
+            new_filter_func = partial(FILTER_FUNCTION_NAMES[filter_name], **filter_args)
+            new_filter_func.__name__ = FILTER_FUNCTION_NAMES[filter_name].__name__
         else:
             raise TypeError("filter_spec should be a string or a tuple or list")
-        check_all_parameters_given(filter_func)
-        self.filters.append(filter_func)
+        check_all_parameters_given(new_filter_func)
+
+        # Replace filters that are already stored.
+        filter_already_added = False
+        for i, filter_function in enumerate(self.filters):
+            if new_filter_func.__name__ == filter_function.__name__:
+                logger.warning("The filter %s was already in the filter list, "
+                               "the last added filter parameters are used, "
+                               "check yaml file for details", new_filter_func)
+                self.filters[i] = new_filter_func
+                filter_already_added = True
+        if not filter_already_added:
+            self.filters.append(new_filter_func)
         # Sort filters according to their order in self.filter_order
         self.filters.sort(key=lambda f: self.filter_order.index(f.__name__))
 
-    def add_custom_filter(self, filter_function: Union[Tuple[Callable, Dict[str, any]], Callable], filter_params=None, filter_position=None):
+    def add_custom_filter(self, filter_function, filter_params=None, filter_position: Optional[int] = None):
         """
         Add a custom filter function to the processing pipeline.
 
@@ -239,9 +253,8 @@ class ProcessingReport:
     """Class to keep track of spectrum changes during filtering.
     """
     def __init__(self):
-        self.counter_changed_metadata = defaultdict(int)
+        self.counter_changed_spectrum = defaultdict(int)
         self.counter_removed_spectrums = defaultdict(int)
-        self.counter_changed_peaks = defaultdict(int)
         self.counter_number_processed = 0
 
     def add_to_report(self, spectrum_old, spectrum_new: Spectrum,
@@ -251,23 +264,18 @@ class ProcessingReport:
         if spectrum_new is None:
             self.counter_removed_spectrums[filter_function_name] += 1
         else:
-            # Add metadata changes
-            if spectrum_new.metadata != spectrum_old.metadata:
-                self.counter_changed_metadata[filter_function_name] += 1
-            # Add peak changes
-            if spectrum_new.peaks != spectrum_old.peaks or spectrum_new.losses != spectrum_old.losses:
-                self.counter_changed_peaks[filter_function_name] += 1
+            for field, value in spectrum_new.metadata.items():
+                if objects_differ(spectrum_old.get(field), value):
+                    self.counter_changed_spectrum[filter_function_name] += 1
+                    break
 
     def to_dataframe(self):
         """Create Pandas DataFrame Report of counted spectrum changes."""
-        metadata_changed = pd.DataFrame(self.counter_changed_metadata.items(),
-                                        columns=["filter", "changed metadata"])
+        changes = pd.DataFrame(self.counter_changed_spectrum.items(),
+                               columns=["filter", "changed spectra"])
         removed = pd.DataFrame(self.counter_removed_spectrums.items(),
                                columns=["filter", "removed spectra"])
-        peaks_changed = pd.DataFrame(self.counter_changed_peaks.items(),
-                                     columns=["filter", "changed mass spectrum"])
-        processing_report = pd.merge(removed, metadata_changed, how="outer", on="filter")
-        processing_report = pd.merge(processing_report, peaks_changed, how="outer", on="filter")
+        processing_report = pd.merge(removed, changes, how="outer", on="filter")
 
         processing_report = processing_report.set_index("filter").fillna(0)
         return processing_report.astype(int)
@@ -285,9 +293,7 @@ Changes during processing:
     def __repr__(self):
         return f"Report({self.counter_number_processed},\
         {self.counter_removed_spectrums},\
-        {dict(self.counter_removed_spectrums)},\
-        {dict(self.counter_changed_metadata)},\
-        {dict(self.counter_changed_peaks)})"
+        {dict(self.counter_changed_spectrum)})"
 
 
 def objects_differ(obj1, obj2):
diff --git a/matchms/filtering/metadata_processing/repair_not_matching_annotation.py b/matchms/filtering/metadata_processing/repair_not_matching_annotation.py
index fbbf3181..cc617eb8 100644
--- a/matchms/filtering/metadata_processing/repair_not_matching_annotation.py
+++ b/matchms/filtering/metadata_processing/repair_not_matching_annotation.py
@@ -48,12 +48,12 @@ def repair_not_matching_annotation(spectrum_in: Spectrum):
         # Repairing is not possible, since not all annotations are valid entries.
         return spectrum
 
-    inchikey_from_smiles = convert_inchi_to_inchikey(convert_smiles_to_inchi(spectrum.get("smiles")))
-    inchikey_from_inchi = convert_inchi_to_inchikey(spectrum.get("inchi"))
+    smiles_from_inchi = convert_inchi_to_smiles(spectrum.get("inchi"))
 
     # Check if SMILES and InChI match
-    if inchikey_from_smiles[:14] != inchikey_from_inchi[:14]:
-        spectrum = _repair_smiles_inchi(spectrum)
+    if smiles_from_inchi != spectrum.get("smiles"):
+        spectrum = _repair_smiles_inchi(spectrum,
+                                        smiles_from_inchi)
 
     # Check if the InChIKey matches the InChI
     correct_inchikey = convert_inchi_to_inchikey(spectrum.get("inchi"))
@@ -90,12 +90,11 @@ def _check_repairing_is_possible(smiles, inchi, inchikey) -> bool:
     return False
 
 
-def _repair_smiles_inchi(spectrum):
+def _repair_smiles_inchi(spectrum, smiles_from_inchi):
     """Repairs mismatching smiles and inchi. The smile or inchi that matches the parent mass is chosen"""
     smiles = spectrum.get("smiles")
     parent_mass = spectrum.get("parent_mass")
     inchi = spectrum.get("inchi")
-    smiles_from_inchi = convert_inchi_to_smiles(inchi)
 
     smiles_correct = _check_smiles_and_parent_mass_match(smiles, parent_mass, 0.1)
     inchi_correct = _check_smiles_and_parent_mass_match(smiles_from_inchi, parent_mass, 0.1)


Test patch:
diff --git a/tests/filtering/test_spectrum_processor.py b/tests/filtering/test_spectrum_processor.py
index 4143f8df..0c91a1e2 100644
--- a/tests/filtering/test_spectrum_processor.py
+++ b/tests/filtering/test_spectrum_processor.py
@@ -1,21 +1,23 @@
 import numpy as np
 import pytest
 from matchms import SpectrumProcessor
+from matchms.filtering.filter_order_and_default_pipelines import BASIC_FILTERS
 from matchms.filtering.SpectrumProcessor import ProcessingReport
 from ..builder_Spectrum import SpectrumBuilder
 
 
 @pytest.fixture
 def spectrums():
-    s1 = SpectrumBuilder().\
-        with_metadata({"charge": "+1", "pepmass": 100}).\
-        with_mz([10, 20, 30]).with_intensities([0.1, 0.4, 10]).build()
-    s2 = SpectrumBuilder().with_metadata({"charge": "-1",
-                                          "pepmass": 102}).\
-        with_mz([10, 20, 30]).with_intensities([0.1, 0.2, 1]).build()
-    s3 = SpectrumBuilder().with_metadata({"charge": -1,
-                                          "pepmass": 104}).\
-        with_mz([10, ]).with_intensities([0.1, ]).build()
+    metadata1 = {"charge": "+1",
+                 "pepmass": 100}
+    metadata2 = {"charge": "-1",
+                 "pepmass": 102}
+    metadata3 = {"charge": -1,
+                 "pepmass": 104}
+
+    s1 = SpectrumBuilder().with_metadata(metadata1).build()
+    s2 = SpectrumBuilder().with_metadata(metadata2).build()
+    s3 = SpectrumBuilder().with_metadata(metadata3).build()
     return [s1, s2, s3]
 
 
@@ -70,7 +72,7 @@ def test_filter_sorting_and_output():
     [("require_correct_ionmode", {"ion_mode_to_keep": "both"}),
      ("require_correct_ionmode", {"ion_mode_to_keep": "both"})],
 ])
-def test_overwrite_default_settings(filter_step, expected):
+def test_overwrite_default_settings(filter_step: str, expected):
     """Test if both default settings and set settings are returned in processing steps"""
     processor = SpectrumProcessor(None)
     processor.add_filter(filter_step)
@@ -134,20 +136,18 @@ def test_filter_spectrums(spectrums):
 
 def test_filter_spectrums_report(spectrums):
     processor = SpectrumProcessor("minimal")
-    processor.add_filter(filter_function=("require_minimum_number_of_peaks", {"n_required": 2}))
-    processor.add_filter(filter_function="add_losses")
     spectrums, report = processor.process_spectrums(spectrums, create_report=True)
-    assert len(spectrums) == 2
+    assert len(spectrums) == 3
     actual_masses = [s.get("precursor_mz") for s in spectrums]
-    expected_masses = [100, 102]
+    expected_masses = [100, 102, 104]
     assert actual_masses == expected_masses
     assert report.counter_number_processed == 3
-    assert report.counter_changed_metadata == {'make_charge_int': 2, 'interpret_pepmass': 3, 'derive_ionmode': 3}
+    assert report.counter_changed_spectrum == {'make_charge_int': 2, 'interpret_pepmass': 3, 'derive_ionmode': 3}
     report_df = report.to_dataframe()
-    assert np.all(report_df.loc[["require_minimum_number_of_peaks", "interpret_pepmass", "add_losses"]].values == np.array(
-        [[1, 0, 0],
-         [0, 3, 0],
-         [0, 0, 2]]))
+    assert np.all(report_df.loc[["make_charge_int", "interpret_pepmass", "derive_ionmode"]].values == np.array(
+        [[0, 2],
+         [0, 3],
+         [0, 3]]))
 
 
 def test_processing_report_class(spectrums):
@@ -158,7 +158,7 @@ def test_processing_report_class(spectrums):
         processing_report.add_to_report(s, spectrum_processed, "test_filter")
 
     assert not processing_report.counter_removed_spectrums
-    assert processing_report.counter_changed_metadata == {"test_filter": 3}
+    assert processing_report.counter_changed_spectrum == {"test_filter": 3}
 
 
 def test_adding_custom_filter(spectrums):
@@ -173,7 +173,7 @@ def test_adding_custom_filter(spectrums):
     assert filters[-1].__name__ == "nonsense_inchikey"
     spectrums, report = processor.process_spectrums(spectrums, create_report=True)
     assert report.counter_number_processed == 3
-    assert report.counter_changed_metadata == {'make_charge_int': 2, 'interpret_pepmass': 3,
+    assert report.counter_changed_spectrum == {'make_charge_int': 2, 'interpret_pepmass': 3,
                                                'derive_ionmode': 3, 'nonsense_inchikey': 3}
     assert spectrums[0].get("inchikey") == "NONSENSE", "Custom filter not executed properly"
 
@@ -190,7 +190,7 @@ def test_adding_custom_filter_with_parameters(spectrums):
     assert filters[-1].__name__ == "nonsense_inchikey_multiple"
     spectrums, report = processor.process_spectrums(spectrums, create_report=True)
     assert report.counter_number_processed == 3
-    assert report.counter_changed_metadata == {'make_charge_int': 2, 'interpret_pepmass': 3,
+    assert report.counter_changed_spectrum == {'make_charge_int': 2, 'interpret_pepmass': 3,
                                                'derive_ionmode': 3, 'nonsense_inchikey_multiple': 3}
     assert spectrums[0].get("inchikey") == "NONSENSENONSENSE", "Custom filter not executed properly"
 
@@ -239,3 +239,20 @@ def test_add_filter_with_matchms_filter(spectrums):
     assert filters[-1].__name__ == "require_correct_ionmode"
     spectrums, _ = processor.process_spectrums(spectrums, create_report=True)
     assert not spectrums, "Expected to be empty list"
+
+
+def test_add_duplicated_filter_to_existing_pipeline():
+    """Tests if adding a filter that is already in the basic pipeline is overwritten and not duplicated"""
+    processor = SpectrumProcessor("basic")
+    duplicated_filter = ("derive_adduct_from_name", {"remove_adduct_from_name": False})
+    processor.add_filter(duplicated_filter)
+    assert len(processor.processing_steps) == len(BASIC_FILTERS), "The duplicated filter was not replaced"
+    assert duplicated_filter in processor.processing_steps, "The new settings of the duplicated filter were not added"
+
+
+def test_add_filter_twice():
+    """Tests if adding a filter that is already in the basic pipeline is overwritten and not duplicated"""
+    processor = SpectrumProcessor(None)
+    processor.add_filter(("derive_adduct_from_name", {"remove_adduct_from_name": False}))
+    processor.add_filter("derive_adduct_from_name")
+    assert processor.processing_steps == [("derive_adduct_from_name", {"remove_adduct_from_name": True})]