Repository: matchms/matchms
Pull request #275 (2021-12-07 09:09:24)

improve string entry handling
- refactored `add_precursor_mz()`
- refactored `ModifiedCosine`
- added logging messages and allow `ModifiedCosine` to also handle string entries for precursor_mz.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 461023ac..060c0eb5 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,9 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Added
 
+- better, more flexible string handling of `ModifiedCosine` [#275](https://github.com/matchms/matchms/pull/275)
 - matchms logger, replacing all former `print` statments to better control logging output [#271](https://github.com/matchms/matchms/pull/271)
 - `add_logging_to_file()`, `set_matchms_logger_level()`, `reset_matchms_logger()` functions to adapt logging output to user needs [#271](https://github.com/matchms/matchms/pull/271) 
 
+## Changed
+
+- refactored `add_precursor_mz`, including better logging [#275](https://github.com/matchms/matchms/pull/275)
+
 ## [0.10.0] - 2021-11-21
 
 ### Added
diff --git a/matchms/filtering/add_precursor_mz.py b/matchms/filtering/add_precursor_mz.py
index cf0b522b..6360e2f6 100644
--- a/matchms/filtering/add_precursor_mz.py
+++ b/matchms/filtering/add_precursor_mz.py
@@ -8,6 +8,25 @@ logger = logging.getLogger("matchms")
 
 _accepted_keys = ["precursor_mz", "precursormz", "precursor_mass"]
 _accepted_types = (float, str, int)
+_accepted_missing_entries = ["", "N/A", "NA", "n/a"]
+
+
+def _convert_precursor_mz(precursor_mz):
+    """Convert precursor_mz to number if possible. Otherwise return None."""
+    if precursor_mz is None:
+        return None
+    if isinstance(precursor_mz, str) and precursor_mz in _accepted_missing_entries:
+        return None
+    if not isinstance(precursor_mz, _accepted_types):
+        logger.warning("Found precursor_mz of undefined type.")
+        return None
+    if isinstance(precursor_mz, str):
+        try:
+            return float(precursor_mz.strip())
+        except ValueError:
+            logger.warning("%s can't be converted to float.", precursor_mz)
+            return None
+    return precursor_mz
 
 
 def add_precursor_mz(spectrum_in: SpectrumType) -> SpectrumType:
@@ -23,20 +42,16 @@ def add_precursor_mz(spectrum_in: SpectrumType) -> SpectrumType:
 
     precursor_mz_key = get_first_common_element(spectrum.metadata.keys(), _accepted_keys)
     precursor_mz = spectrum.get(precursor_mz_key)
-
-    if isinstance(precursor_mz, _accepted_types):
-        if isinstance(precursor_mz, str):
-            try:
-                precursor_mz = float(precursor_mz.strip())
-            except ValueError:
-                logger.warning("%s can't be converted to float.", precursor_mz)
-                return spectrum
+    precursor_mz = _convert_precursor_mz(precursor_mz)
+    if isinstance(precursor_mz, (float, int)):
         spectrum.set("precursor_mz", float(precursor_mz))
-    elif precursor_mz is None:
-        pepmass = spectrum.get("pepmass")
-        if pepmass is not None and isinstance(pepmass[0], float):
-            spectrum.set("precursor_mz", pepmass[0])
-        else:
-            logger.warning("No precursor_mz found in metadata.")
+        return spectrum
+
+    pepmass = spectrum.get("pepmass", None)
+    if pepmass is not None and _convert_precursor_mz(pepmass[0]) is not None:
+        spectrum.set("precursor_mz", pepmass[0])
+        logger.info("Added precursor_mz entry based on field 'pepmass'.")
+        return spectrum
 
+    logger.warning("No precursor_mz found in metadata.")
     return spectrum
diff --git a/matchms/similarity/ModifiedCosine.py b/matchms/similarity/ModifiedCosine.py
index 4049c687..c344fdd9 100644
--- a/matchms/similarity/ModifiedCosine.py
+++ b/matchms/similarity/ModifiedCosine.py
@@ -1,11 +1,16 @@
+import logging
 from typing import Tuple
 import numpy
+from matchms.filtering.add_precursor_mz import _convert_precursor_mz
 from matchms.typing import SpectrumType
 from .BaseSimilarity import BaseSimilarity
 from .spectrum_similarity_functions import collect_peak_pairs
 from .spectrum_similarity_functions import score_best_matches
 
 
+logger = logging.getLogger("matchms")
+
+
 class ModifiedCosine(BaseSimilarity):
     """Calculate 'modified cosine score' between mass spectra.
 
@@ -84,21 +89,32 @@ class ModifiedCosine(BaseSimilarity):
 
         Tuple with cosine score and number of matched peaks.
         """
+        def get_valid_precursor_mz(spectrum):
+            """Extract valid precursor_mz from spectrum if possible. If not raise exception."""
+            message_precursor_missing = \
+                "Precursor_mz missing. Apply 'add_precursor_mz' filter first."
+            message_precursor_no_number = \
+                "Precursor_mz must be of type int or float. Apply 'add_precursor_mz' filter first."
+            message_precursor_below_0 = "Expect precursor to be positive number." \
+                                        "Apply 'require_precursor_mz' first"
+
+            precursor_mz = spectrum.get("precursor_mz", None)
+            if not isinstance(precursor_mz, (int, float)):
+                logger.warning(message_precursor_no_number)
+            precursor_mz = _convert_precursor_mz(precursor_mz)
+            assert precursor_mz is not None, message_precursor_missing
+            assert precursor_mz > 0, message_precursor_below_0
+            return precursor_mz
+
         def get_matching_pairs():
             """Find all pairs of peaks that match within the given tolerance."""
             zero_pairs = collect_peak_pairs(spec1, spec2, self.tolerance, shift=0.0,
                                             mz_power=self.mz_power,
                                             intensity_power=self.intensity_power)
-            message_precursor_missing = \
-                "Precursor_mz missing. Apply 'add_precursor_mz' filter first."
-            assert reference.get("precursor_mz") \
-                   and query.get("precursor_mz"), message_precursor_missing
-            message_precursor_below_0 = "Expect precursor to be positive number." \
-                                        "Apply 'require_precursor_mz' first"
-            assert reference.get("precursor_mz") > 0 \
-                   and query.get("precursor_mz") > 0, message_precursor_below_0
+            precursor_mz_ref = get_valid_precursor_mz(reference)
+            precursor_mz_query = get_valid_precursor_mz(query)
 
-            mass_shift = reference.get("precursor_mz") - query.get("precursor_mz")
+            mass_shift = precursor_mz_ref - precursor_mz_query
             nonzero_pairs = collect_peak_pairs(spec1, spec2, self.tolerance, shift=mass_shift,
                                                mz_power=self.mz_power,
                                                intensity_power=self.intensity_power)


Test patch:
diff --git a/tests/test_add_precursor_mz.py b/tests/test_add_precursor_mz.py
index e644159b..9a15e38a 100644
--- a/tests/test_add_precursor_mz.py
+++ b/tests/test_add_precursor_mz.py
@@ -2,6 +2,8 @@ import numpy
 import pytest
 from matchms import Spectrum
 from matchms.filtering import add_precursor_mz
+from matchms.logging_functions import reset_matchms_logger
+from matchms.logging_functions import set_matchms_logger_level
 
 
 def test_add_precursor_mz():
@@ -32,8 +34,9 @@ def test_add_precursor_mz_no_masses():
     assert spectrum.get("precursor_mz") is None, "Outcome should be None."
 
 
-def test_add_precursor_mz_only_pepmass_present():
+def test_add_precursor_mz_only_pepmass_present(caplog):
     """Test if precursor_mz is correctly derived if only pepmass is present."""
+    set_matchms_logger_level("INFO")
     mz = numpy.array([], dtype='float')
     intensities = numpy.array([], dtype='float')
     metadata = {"pepmass": (444.0, 10)}
@@ -44,6 +47,9 @@ def test_add_precursor_mz_only_pepmass_present():
     spectrum = add_precursor_mz(spectrum_in)
 
     assert spectrum.get("precursor_mz") == 444.0, "Expected different precursor_mz."
+    assert "Added precursor_mz entry based on field 'pepmass'" in caplog.text, \
+        "Expected different log message"
+    reset_matchms_logger()
 
 
 @pytest.mark.parametrize("key, value, expected", [
@@ -70,6 +76,26 @@ def test_add_precursor_mz_no_precursor_mz(key, value, expected):
     assert spectrum.get("precursor_mz") == expected, "Expected different precursor_mz."
 
 
+@pytest.mark.parametrize("key, value, expected_log", [
+    ["precursor_mz", "N/A", "No precursor_mz found in metadata."],
+    ["precursor_mass", "test", "test can't be converted to float."],
+    ["precursor_mz", None, "No precursor_mz found in metadata."],
+    ["pepmass", None, "No precursor_mz found in metadata."],
+    ["precursor_mz", [], "Found precursor_mz of undefined type."]])
+def test_add_precursor_mz_logging(key, value, expected_log, caplog):
+    """Test if precursor_mz is correctly derived if "precursor_mz" is str."""
+    mz = numpy.array([], dtype='float')
+    intensities = numpy.array([], dtype='float')
+    metadata = {key: value}
+    spectrum_in = Spectrum(mz=mz,
+                           intensities=intensities,
+                           metadata=metadata)
+
+    _ = add_precursor_mz(spectrum_in)
+
+    assert expected_log in caplog.text, "Expected different logging message."
+
+
 def test_empty_spectrum():
     spectrum_in = None
     spectrum = add_precursor_mz(spectrum_in)
diff --git a/tests/test_modified_cosine.py b/tests/test_modified_cosine.py
index e5b0adcd..d3fe7154 100644
--- a/tests/test_modified_cosine.py
+++ b/tests/test_modified_cosine.py
@@ -121,3 +121,44 @@ def test_modified_cosine_with_mass_shift_5_no_matches_expected():
 
     assert score["score"] == pytest.approx(0.0, 1e-5), "Expected different modified cosine score."
     assert score["matches"] == 0, "Expected 0 matching peaks."
+
+
+def test_modified_cosine_precursor_mz_as_invalid_string():
+    """Test modified cosine on two spectra with precursor_mz given as string."""
+    spectrum_1 = Spectrum(mz=numpy.array([100, 200, 300], dtype="float"),
+                          intensities=numpy.array([10, 10, 500], dtype="float"),
+                          metadata={"precursor_mz": 1000.0})
+
+    spectrum_2 = Spectrum(mz=numpy.array([120, 220, 320], dtype="float"),
+                          intensities=numpy.array([10, 10, 500], dtype="float"),
+                          metadata={"precursor_mz": "mz 1005.0"})
+
+    norm_spectrum_1 = normalize_intensities(spectrum_1)
+    norm_spectrum_2 = normalize_intensities(spectrum_2)
+    modified_cosine = ModifiedCosine(tolerance=1.0)
+    with pytest.raises(AssertionError) as msg:
+        _ = modified_cosine.pair(norm_spectrum_1, norm_spectrum_2)
+
+    expected_message = "Precursor_mz missing. Apply 'add_precursor_mz' filter first."
+    assert str(msg.value) == expected_message
+
+
+def test_modified_cosine_precursor_mz_as_string(caplog):
+    """Test modified cosine on two spectra with precursor_mz given as string."""
+    spectrum_1 = Spectrum(mz=numpy.array([100, 200, 300], dtype="float"),
+                          intensities=numpy.array([10, 10, 500], dtype="float"),
+                          metadata={"precursor_mz": 1000.0})
+
+    spectrum_2 = Spectrum(mz=numpy.array([120, 220, 320], dtype="float"),
+                          intensities=numpy.array([10, 10, 500], dtype="float"),
+                          metadata={"precursor_mz": "1005.0"})
+
+    norm_spectrum_1 = normalize_intensities(spectrum_1)
+    norm_spectrum_2 = normalize_intensities(spectrum_2)
+    modified_cosine = ModifiedCosine(tolerance=1.0)
+    score = modified_cosine.pair(norm_spectrum_1, norm_spectrum_2)
+
+    assert score["score"] == pytest.approx(0.0, 1e-5), "Expected different modified cosine score."
+    assert score["matches"] == 0, "Expected 0 matching peaks."
+    expected_msg = "Precursor_mz must be of type int or float. Apply 'add_precursor_mz' filter first."
+    assert expected_msg in caplog.text, "Expected different log message"