Repository: matchms/matchms
Pull request #523 (2023-09-06 10:12:24)

Improve derive adduct from name
Some compound names had multiple adducts in the name. These were previously not removed. Now improve_derive_adduct_form_name does remove every occurance of an adduct and adds only an adduct if it can actually be interpreted to retrieve a mass_multiplier and a charge. 

@florian-huber Did I miss any type of common input for which this approach would not work well? 


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/filter_order_and_default_pipelines.py b/matchms/filtering/filter_order_and_default_pipelines.py
index 175f018e..814fce05 100644
--- a/matchms/filtering/filter_order_and_default_pipelines.py
+++ b/matchms/filtering/filter_order_and_default_pipelines.py
@@ -13,8 +13,6 @@ ALL_FILTERS = [msfilters.make_charge_int,
                msfilters.add_retention_time,
                msfilters.derive_ionmode,
                msfilters.correct_charge,
-               # msfilters.derive_adduct_from_name,  # run again? Or improve those filters?
-               # msfilters.derive_formula_from_name,  # run again? Or improve those filters?
                msfilters.require_precursor_mz,
                msfilters.harmonize_undefined_inchikey,
                msfilters.harmonize_undefined_inchi,
diff --git a/matchms/filtering/metadata_processing/derive_adduct_from_name.py b/matchms/filtering/metadata_processing/derive_adduct_from_name.py
index 8eea01a5..e48668aa 100644
--- a/matchms/filtering/metadata_processing/derive_adduct_from_name.py
+++ b/matchms/filtering/metadata_processing/derive_adduct_from_name.py
@@ -1,6 +1,9 @@
 import logging
 import re
-from matchms.typing import SpectrumType
+from typing import List, Optional
+from matchms.filtering.filter_utils.interpret_unknown_adduct import \
+    get_multiplier_and_mass_from_adduct
+from matchms.Spectrum import Spectrum
 from ..filter_utils.load_known_adducts import load_known_adducts
 from .clean_adduct import _clean_adduct
 
@@ -8,8 +11,8 @@ from .clean_adduct import _clean_adduct
 logger = logging.getLogger("matchms")
 
 
-def derive_adduct_from_name(spectrum_in: SpectrumType,
-                            remove_adduct_from_name: bool = True) -> SpectrumType:
+def derive_adduct_from_name(spectrum_in: Spectrum,
+                            remove_adduct_from_name: bool = True) -> Optional[Spectrum]:
     """Find adduct in compound name and add to metadata (if not present yet).
 
     Method to interpret the given compound name to find the adduct.
@@ -26,36 +29,57 @@ def derive_adduct_from_name(spectrum_in: SpectrumType,
 
     spectrum = spectrum_in.clone()
 
-    if spectrum.get("compound_name", None) is not None:
-        name = spectrum.get("compound_name")
-    else:
-        assert spectrum.get("name", None) in [None, ""], ("Found 'name' but not 'compound_name' in metadata",
-                                                          "Apply 'add_compound_name' filter first.")
+    compound_name = spectrum.get("compound_name", None)
+    if compound_name is None:
+        if spectrum.get("name", None) not in [None, ""]:
+            logger.warning("Found 'name' but not 'compound_name' in metadata"
+                           "Apply 'add_compound_name' filter first.")
         return spectrum
-
     # Detect adduct in compound name
-    adduct_from_name = None
-    name_split = name.split(" ")
-    for name_part in name_split[::-1][:2]:
+    parts_that_look_like_adduct = []
+    name_split = compound_name.split(" ")
+    for name_part in name_split:
         if _looks_like_adduct(name_part):
-            adduct_from_name = name_part
-            break
+            # Some adducts occur more than once. So they are all removed.
+            parts_that_look_like_adduct.append(name_part)
 
-    if adduct_from_name and remove_adduct_from_name:
-        name_adduct_removed = " ".join([x for x in name_split if x != adduct_from_name])
+    if remove_adduct_from_name and len(parts_that_look_like_adduct) > 0:
+        name_adduct_removed = " ".join([x for x in name_split if x not in parts_that_look_like_adduct])
         name_adduct_removed = name_adduct_removed.strip("; ")
         spectrum.set("compound_name", name_adduct_removed)
-        logger.info("Removed adduct %s from compound name.", adduct_from_name)
+        logger.info("Removed adduct %s from compound name.", parts_that_look_like_adduct)
 
-    # Add found adduct to metadata (if not present yet)
-    if adduct_from_name and not _looks_like_adduct(spectrum.get("adduct")):
-        adduct_cleaned = _clean_adduct(adduct_from_name)
-        spectrum.set("adduct", adduct_cleaned)
-        logger.info("Added adduct %s from the compound name to metadata.", spectrum.get('adduct'))
+    if len(parts_that_look_like_adduct) > 0 and not _looks_like_adduct(spectrum.get("adduct")):
+        best_adduct = _select_best_adduct(parts_that_look_like_adduct)
+        if best_adduct:
+            # Add found adduct to metadata (if not present yet)
+            spectrum.set("adduct", best_adduct)
+            logger.info("Added adduct %s from the compound name to metadata.", spectrum.get('adduct'))
 
     return spectrum
 
 
+def _select_best_adduct(list_of_adducts: List[str]) -> Optional[str]:
+    """Selects an adduct that can actually be interpreted (complete with charge and known elements)"""
+    unique_cleaned_adducts = list({_clean_adduct(adduct) for adduct in list_of_adducts})
+    if len(unique_cleaned_adducts) == 1:
+        return unique_cleaned_adducts[0]
+
+    completely_correct_adduct = []
+    for adduct in unique_cleaned_adducts:
+        multiplier, correction_mass = get_multiplier_and_mass_from_adduct(adduct)
+        # check if both multiplier and correction mass are not None
+        if multiplier and correction_mass:
+            completely_correct_adduct.append(adduct)
+    if len(completely_correct_adduct) == 0:
+        return None
+    if len(completely_correct_adduct) == 1:
+        return completely_correct_adduct[0]
+    logger.warning("Two potential adducts were found in the compound name that are both valid adducts. "
+                   "The first adduct is used. The adducts found are: %s", completely_correct_adduct)
+    return completely_correct_adduct[0]
+
+
 def _looks_like_adduct(adduct):
     """Return True if input string has expected format of an adduct."""
     if not isinstance(adduct, str):
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_all_filter_order.py b/tests/filtering/test_all_filter_order.py
index 13e62c19..f8d76ba9 100644
--- a/tests/filtering/test_all_filter_order.py
+++ b/tests/filtering/test_all_filter_order.py
@@ -29,7 +29,10 @@ ANNOTATION_REPARATIONS = [msfilters.repair_inchi_inchikey_smiles, msfilters.deri
     [ANNOTATION_REPARATIONS + [msfilters.add_parent_mass],
      [msfilters.derive_smiles_from_pubchem_compound_name_search, ]],
     # The parent mass is based on the adduct, so adduct filters should be performed first
-    [[msfilters.clean_adduct, msfilters.derive_adduct_from_name], [msfilters.add_parent_mass]]
+    [[msfilters.clean_adduct, msfilters.derive_adduct_from_name], [msfilters.add_parent_mass]],
+    # The adduct filter removes all occurances while derive formula from name only removes when it is at the end
+    # of compound name. Removing adducts therefore has to happen first.
+    [[msfilters.derive_adduct_from_name], [msfilters.derive_formula_from_name]],
 ])
 def test_all_filter_order(early_filters: List[Callable], later_filters: List[Callable]):
     """Tests if early_filter is run before later_filter"""
diff --git a/tests/filtering/test_derive_adduct_from_name.py b/tests/filtering/test_derive_adduct_from_name.py
index 8b8ce5a0..1ef49223 100644
--- a/tests/filtering/test_derive_adduct_from_name.py
+++ b/tests/filtering/test_derive_adduct_from_name.py
@@ -14,9 +14,11 @@ from ..builder_Spectrum import SpectrumBuilder
     [{"compound_name": "peptideXYZ [M+H+K]", "adduct": "M+H"}, True, "M+H", "peptideXYZ", "[M+H+K]"],
     [{"compound_name": "peptideXYZ [M+H+K]"}, False, "[M+H+K]", "peptideXYZ [M+H+K]", None],
     [{"Name": "peptideXYZ [M+H+K]", "adduct": "M+H"}, True, "M+H", "peptideXYZ", "[M+H+K]"],
+    [{"compound_name": "peptideXYZ [M+H+K] C16H12"}, True, "[M+H+K]", "peptideXYZ C16H12", "[M+H+K]"],
     [{"name": ""}, True, None, "", None]
 ])
-def test_derive_adduct_from_name_parametrized(metadata, remove_adduct_from_name, expected_adduct, expected_name, removed_adduct):
+def test_derive_adduct_from_name_parametrized(metadata, remove_adduct_from_name,
+                                              expected_adduct, expected_name, removed_adduct):
     set_matchms_logger_level("INFO")
     spectrum_in = SpectrumBuilder().with_metadata(metadata).build()
 
@@ -28,7 +30,7 @@ def test_derive_adduct_from_name_parametrized(metadata, remove_adduct_from_name,
 
     expected_log = []
     if spectrum.get("compound_name") != spectrum_in.get("compound_name"):
-        expected_log.append(('matchms', 'INFO', f'Removed adduct {removed_adduct} from compound name.'))
+        expected_log.append(('matchms', 'INFO', f"Removed adduct ['{removed_adduct}'] from compound name."))
     if spectrum.get("adduct") != spectrum_in.get("adduct"):
         expected_log.append(('matchms', 'INFO', f'Added adduct {expected_adduct} from the compound name to metadata.'))
 
@@ -36,6 +38,26 @@ def test_derive_adduct_from_name_parametrized(metadata, remove_adduct_from_name,
     reset_matchms_logger()
 
 
+@pytest.mark.parametrize("metadata, expected_adduct, expected_name", [
+    # Test removing two adducts and selecting the interpretable one
+    [{"compound_name": "peptideXYZ [M+H+K] C16H12 [M+H+K]2+"}, "[M+H+K]2+", "peptideXYZ C16H12"],
+    # Test removing two adducts and not adding if not interpretable
+    [{"compound_name": "peptideXYZ [M+H+K] C16H12 [M+2H+K]"}, None, "peptideXYZ C16H12"],
+    # Test removing two adducts and check that clean adduct is run (to recover known adducts)
+    [{"compound_name": "peptideXYZ [M+H+K] C16H12 [M+H]"}, "[M+H]+", "peptideXYZ C16H12"]
+
+])
+def test_derive_adduct_from_name_multiple_adducts_in_name(metadata,
+                                                          expected_adduct, expected_name):
+    set_matchms_logger_level("INFO")
+    spectrum_in = SpectrumBuilder().with_metadata(metadata).build()
+
+    spectrum = derive_adduct_from_name(spectrum_in, remove_adduct_from_name=True)
+
+    assert spectrum.get("adduct") == expected_adduct, "Expected different adduct."
+    assert spectrum.get("compound_name") == expected_name, "Expected different cleaned name."
+
+
 def test_empty_spectrum():
     spectrum_in = None
     spectrum = derive_adduct_from_name(spectrum_in)
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"