Repository: matchms/matchms
Pull request #522 (2023-09-05 12:37:09)

Further cleaning repair_not_matching_annotation
@florian-huber Because of your cleaning in #521 I had a second look at the structure and made some additional changes. 
Feel free to merge if you agree with the changes. 


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..3c0c2659 100644
--- a/matchms/filtering/SpectrumProcessor.py
+++ b/matchms/filtering/SpectrumProcessor.py
@@ -1,7 +1,7 @@
 import inspect
 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
@@ -35,8 +35,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.
         """
@@ -74,7 +73,7 @@ class SpectrumProcessor:
         # 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 +238,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 +249,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 +278,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 4cd7ee61..fbbf3181 100644
--- a/matchms/filtering/metadata_processing/repair_not_matching_annotation.py
+++ b/matchms/filtering/metadata_processing/repair_not_matching_annotation.py
@@ -1,7 +1,7 @@
 import logging
 from matchms import Spectrum
 from matchms.filtering.filter_utils.smile_inchi_inchikey_conversions import (
-    _check_fully_annotated, convert_inchi_to_inchikey, convert_inchi_to_smiles,
+    convert_inchi_to_inchikey, convert_inchi_to_smiles,
     convert_smiles_to_inchi, is_valid_inchi, is_valid_inchikey,
     is_valid_smiles)
 from matchms.filtering.metadata_processing.require_parent_mass_match_smiles import \
@@ -44,47 +44,59 @@ def repair_not_matching_annotation(spectrum_in: Spectrum):
 
     spectrum = spectrum_in.clone()
 
-    smiles = spectrum.get("smiles")
-    inchi = spectrum.get("inchi")
-    inchikey = spectrum.get("inchikey")
-
-    if not _check_fully_annotated(spectrum):
-        if is_valid_inchi(inchi) or is_valid_smiles(smiles) or is_valid_inchikey(inchikey):
-            logger.warning("Please first run repair_inchi_from_smiles, repair_smiles_from_inchi and repair_inchikey. "
-                           "The spectrum had partly valid annotations, "
-                           "this shows that these repair functions were not yet run.")
-        else:
-            logger.info("No valid annotation was available for the spectrum, "
-                        "so repair_not_matching_annotation was not run")
+    if not _check_repairing_is_possible(spectrum.get("smiles"), spectrum.get("inchi"), spectrum.get("inchikey")):
+        # Repairing is not possible, since not all annotations are valid entries.
         return spectrum
 
-    parent_mass = spectrum.get("parent_mass")
-    inchi_from_smiles = convert_smiles_to_inchi(smiles)
+    inchikey_from_smiles = convert_inchi_to_inchikey(convert_smiles_to_inchi(spectrum.get("smiles")))
+    inchikey_from_inchi = convert_inchi_to_inchikey(spectrum.get("inchi"))
 
     # Check if SMILES and InChI match
-    if inchi_from_smiles != inchi:
-        smiles_from_inchi = convert_inchi_to_smiles(inchi)
-        spectrum = _repair_smiles_inchi(spectrum,
-                                        smiles,
-                                        inchi,
-                                        smiles_from_inchi,
-                                        inchi_from_smiles,
-                                        parent_mass)
+    if inchikey_from_smiles[:14] != inchikey_from_inchi[:14]:
+        spectrum = _repair_smiles_inchi(spectrum)
 
     # Check if the InChIKey matches the InChI
     correct_inchikey = convert_inchi_to_inchikey(spectrum.get("inchi"))
-    if correct_inchikey and (inchikey[:14] == correct_inchikey[:14]):
+    if correct_inchikey and (spectrum.get("inchikey")[:14] == correct_inchikey[:14]):
         return spectrum
 
-    logger.info("The inchikey has been changed from %s to %s", inchikey, correct_inchikey)
+    logger.info("The inchikey has been changed from %s to %s", spectrum.get("inchikey"), correct_inchikey)
     spectrum.set("inchikey", correct_inchikey)
     return spectrum
 
 
-def _repair_smiles_inchi(spectrum, smiles, inchi,
-                         smiles_from_inchi, inchi_from_smiles,
-                         parent_mass):
-    # pylint: disable=too-many-arguments
+def _check_repairing_is_possible(smiles, inchi, inchikey) -> bool:
+    """Returns True if smiles, inchi and inchikey all are valid.
+
+    If False the appropriate logging message is given.
+    """
+    valid_smiles = is_valid_smiles(smiles)
+    valid_inchi = is_valid_inchi(inchi)
+    valid_inchikey = is_valid_inchikey(inchikey)
+    if valid_smiles and valid_inchi and valid_inchikey:
+        # All annotation is valid, however the different annotations, might not correspond to the same compound.
+        # Therefore repair_not_matching_annotation can be run.
+        return True
+    if not valid_smiles and not valid_inchi and not valid_inchikey:
+        # There is no annotation available
+        logger.info("No valid annotation was available for the spectrum, "
+                    "so repair_not_matching_annotation was not run")
+    elif valid_smiles or valid_inchi or valid_inchikey:
+        # At least one of the annotations, but some are not.
+        # Since if valid_smiles and valid_inchi and valid_inchikey was False
+        logger.warning("Please first run repair_inchi_from_smiles, repair_smiles_from_inchi and repair_inchikey. "
+                       "The spectrum had partly valid annotations, "
+                       "this shows that these repair functions were not yet run.")
+    return False
+
+
+def _repair_smiles_inchi(spectrum):
+    """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)
 
@@ -92,6 +104,7 @@ def _repair_smiles_inchi(spectrum, smiles, inchi,
         logger.warning("The SMILES and InChI are not matching, but both match the parent mass. "
                        "SMILES = %s, InChI = %s", smiles, inchi)
     elif smiles_correct and not inchi_correct:
+        inchi_from_smiles = convert_smiles_to_inchi(spectrum.get("smiles"))
         # Repair by using inchi generated from SMILES
         logger.info("The InChI has been changed from %s to %s. "
                     "The new InChI matches the parent mass, while the old one did not", inchi, inchi_from_smiles)


Test patch:
diff --git a/tests/filtering/test_spectrum_processor.py b/tests/filtering/test_spectrum_processor.py
index 4143f8df..25b39d39 100644
--- a/tests/filtering/test_spectrum_processor.py
+++ b/tests/filtering/test_spectrum_processor.py
@@ -7,15 +7,16 @@ 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]
 
 
@@ -134,20 +135,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 +157,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 +172,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 +189,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"