Repository: matchms/matchms
Pull request #553 (2023-11-09 10:47:57)

Fix retention time for msp
Solves #551 

The issue was that the regex would search for patterns like:

100 s
100 min

But it would use a split function later to find these cases. This resulted in 100 sec, which matches 100 s so passed the regex test, but sec would be used instead.

Fixes:

- [x] Change regex to require end of line as well.
- [x] Added sec as option
- [x] Renaming of some unclear variables
- [x] Added test
- [x] In metadata harmonization all common keys are used, instead of only "retention_time"




Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5153e9d4..b0c265ed 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Add saving function in SpectrumProcessor [#543](https://github.com/matchms/matchms/pull/543)
 ### Fixed
 - Handle missing `precursor_mz` in representation and [#452](https://github.com/matchms/matchms/issues/452) introduced by [#514](https://github.com/matchms/matchms/pull/514/files)[#540](https://github.com/matchms/matchms/pull/540)
+- Fixed retention time harmonization for msp files [#551](https://github.com/matchms/matchms/issues/551)
 - Fix closing mgf file after loading and prevent reopening. [#555](https://github.com/matchms/matchms/issues/555)
 
 ### Changed
diff --git a/matchms/Metadata.py b/matchms/Metadata.py
index 190d90e6..ad3e6862 100644
--- a/matchms/Metadata.py
+++ b/matchms/Metadata.py
@@ -3,7 +3,8 @@ import numpy as np
 from pickydict import PickyDict
 from .filtering.metadata_processing.add_precursor_mz import \
     _add_precursor_mz_metadata
-from .filtering.metadata_processing.add_retention import _add_retention
+from .filtering.metadata_processing.add_retention import (
+    _add_retention, _retention_index_keys, _retention_time_keys)
 from .filtering.metadata_processing.interpret_pepmass import \
     _interpret_pepmass_metadata
 from .filtering.metadata_processing.make_charge_int import \
@@ -106,11 +107,11 @@ class Metadata:
             metadata_filtered["ionmode"] = self.get("ionmode").lower()
 
         if metadata_filtered.get("retention_time"):
-            metadata_filtered = _add_retention(metadata_filtered, "retention_time", "retention_time")
+            metadata_filtered = _add_retention(metadata_filtered, "retention_time", _retention_time_keys)
 
         if metadata_filtered.get("retention_index"):
-            metadata_filtered = _add_retention(metadata_filtered, "retention_index", "retention_index")
-        
+            metadata_filtered = _add_retention(metadata_filtered, "retention_index", _retention_index_keys)
+
         if metadata_filtered.get("parent"):
             metadata_filtered["parent"] = float(metadata_filtered.get("parent"))
 
@@ -121,7 +122,7 @@ class Metadata:
 
         invalid_entries = ["", "NA", "N/A", "NaN"]
         metadata_filtered = {k:v for k,v in metadata_filtered.items() if v not in invalid_entries}
-        
+
         self.data = metadata_filtered
 
     # ------------------------------
@@ -195,7 +196,7 @@ class Metadata:
                 self.harmonize_keys()
         else:
             raise TypeError("Expected input of type dict or PickyDict.")
-    
+
     @staticmethod
     def set_key_replacements(keys: dict):
         """Set key replacements for metadata harmonization.
@@ -205,4 +206,4 @@ class Metadata:
         keys:
             Dictionary with key replacements.
         """
-        Metadata._key_replacements = keys
\ No newline at end of file
+        Metadata._key_replacements = keys
diff --git a/matchms/filtering/metadata_processing/add_retention.py b/matchms/filtering/metadata_processing/add_retention.py
index b109a32a..e8feb218 100644
--- a/matchms/filtering/metadata_processing/add_retention.py
+++ b/matchms/filtering/metadata_processing/add_retention.py
@@ -35,7 +35,7 @@ def _safe_store_value(metadata: dict, value: Any, target_key: str) -> dict:
     return metadata
 
 
-def _safe_convert_to_float(value: Any) -> Optional[float]:
+def _safe_convert_to_float(retention_time: Any) -> Optional[float]:
     """Safely convert value to float. Return 'None' on failure.
 
     Parameters
@@ -47,27 +47,28 @@ def _safe_convert_to_float(value: Any) -> Optional[float]:
     -------
     Converted float value or 'None' if conversion is not possible.
     """
-    if isinstance(value, list):
-        if len(value) == 1:
-            value = value[0]
+    if isinstance(retention_time, list):
+        if len(retention_time) == 1:
+            retention_time = retention_time[0]
         else:
             return None
 
     # logic to read MoNA msp files which specify rt as string with "min" in it
-    if isinstance(value, str):
-        value = value.strip()
-        pattern = r'^[+-]?(\d*\.)?\d+\s*(min|s|h|ms)'
-        conversion = {"min": 60, "s": 1, "h": 3600, "ms": 1e-3}
-        match = re.search(pattern, value)
+    if isinstance(retention_time, str):
+        retention_time = retention_time.strip()
+        pattern = r'^([+-]?\d*\.?\d+)\s*(min|s|h|ms|sec)$'
+        conversion = {"min": 60, "s": 1, "h": 3600, "ms": 1e-3, "sec": 1}
+        match = re.search(pattern, retention_time)
 
         if match and len(match.groups()) == 2:
-            val, unit = value.split(' ')
-            return float(val) * conversion[unit]
+            value = match.group(1)
+            unit = match.group(2)
+            return float(value) * conversion[unit]
     try:
-        value = float(value)
-        rt = value if value >= 0 else None  # discard negative RT values
+        retention_time = float(retention_time)
+        rt = retention_time if retention_time >= 0 else None  # discard negative RT values
     except (ValueError, TypeError):
-        logger.warning("%s can't be converted to float.", str(value))
+        logger.warning("%s can't be converted to float.", str(retention_time))
         rt = None
     return rt
 


Test patch:
diff --git a/tests/filtering/test_add_retention.py b/tests/filtering/test_add_retention.py
index 493bac4a..88de8d18 100644
--- a/tests/filtering/test_add_retention.py
+++ b/tests/filtering/test_add_retention.py
@@ -1,64 +1,65 @@
-import pytest
-from matchms.filtering import add_retention_index, add_retention_time
-from ..builder_Spectrum import SpectrumBuilder
-
-
-@pytest.mark.parametrize("metadata, expected", [
-    [{"retention_time": 100.0}, 100.0],
-    [{"retention_time": "NA"}, None],
-    [{"retention_time": "100.0"}, 100.0],
-    [{"retentiontime": 200}, 200.0],
-    [{"retentiontime": -1}, None],
-    [{"retentiontime": "-1"}, None],
-    [{"rt": "4.810467 min"}, 288.62802],
-    [{"rt": "no retention time in min available"}, None],
-    [{"rt": 200}, 200.0],
-    [{"RT": 200}, 200.0],
-    [{"RT_Query": 200}, 200.0],
-    [{"nothing": "200"}, None],
-    [{'scan_start_time': 0.629566}, 0.629566],
-    [{'scan_start_time': [0.629566]}, 0.629566],
-    [{"rt": None, "retentiontime": 12.17}, 12.17]
-])
-def test_add_retention_time(metadata, expected):
-    spectrum_in = SpectrumBuilder().with_metadata(metadata).build()
-
-    spectrum = add_retention_time(spectrum_in)
-    actual = spectrum.get("retention_time")
-
-    if expected is None:
-        assert actual is None
-    else:
-        assert actual == expected and isinstance(actual, float)
-
-
-@pytest.mark.parametrize("metadata, expected", [
-    [{"retention_index": 100.0}, 100.0],
-    [{"retention_index": "NA"}, None],
-    [{"retention_index": "100.0"}, 100.0],
-    [{"retentionindex": 200}, 200.0],
-    [{"retentionindex": -1}, None],
-    [{"retentionindex": "-1"}, None],
-    [{"ri": 200}, 200.0],
-    [{"RI": 200}, 200.0],
-    [{"nothing": "200"}, None]
-])
-def test_add_retention_index(metadata, expected):
-    spectrum_in = SpectrumBuilder().with_metadata(metadata).build()
-
-    spectrum = add_retention_index(spectrum_in)
-    actual = spectrum.get("retention_index")
-
-    if expected is None:
-        assert actual is None
-    else:
-        assert actual == expected and isinstance(actual, float)
-
-
-def test_empty_spectrum():
-    spectrum_in = None
-    spectrum = add_retention_time(spectrum_in)
-    assert spectrum is None, "Expected different handling of None spectrum."
-
-    spectrum = add_retention_index(spectrum_in)
-    assert spectrum is None, "Expected different handling of None spectrum."
+import pytest
+from matchms.filtering import add_retention_index, add_retention_time
+from ..builder_Spectrum import SpectrumBuilder
+
+
+@pytest.mark.parametrize("metadata, expected", [
+    [{"retention_time": 100.0}, 100.0],
+    [{"retention_time": "NA"}, None],
+    [{"retention_time": "100.0"}, 100.0],
+    [{"retentiontime": 200}, 200.0],
+    [{"retentiontime": -1}, None],
+    [{"retentiontime": "-1"}, None],
+    [{"rt": "4.810467 min"}, 288.62802],
+    [{"rt": "no retention time in min available"}, None],
+    [{"rt": 200}, 200.0],
+    [{"RT": 200}, 200.0],
+    [{"RT_Query": 200}, 200.0],
+    [{"nothing": "200"}, None],
+    [{'scan_start_time': 0.629566}, 0.629566],
+    [{'scan_start_time': [0.629566]}, 0.629566],
+    [{"rt": None, "retentiontime": 12.17}, 12.17],
+    [{"retention_time": "100.0 sec"}, 100.0]
+])
+def test_add_retention_time(metadata, expected):
+    spectrum_in = SpectrumBuilder().with_metadata(metadata).build()
+
+    spectrum = add_retention_time(spectrum_in)
+    actual = spectrum.get("retention_time")
+
+    if expected is None:
+        assert actual is None
+    else:
+        assert actual == expected and isinstance(actual, float)
+
+
+@pytest.mark.parametrize("metadata, expected", [
+    [{"retention_index": 100.0}, 100.0],
+    [{"retention_index": "NA"}, None],
+    [{"retention_index": "100.0"}, 100.0],
+    [{"retentionindex": 200}, 200.0],
+    [{"retentionindex": -1}, None],
+    [{"retentionindex": "-1"}, None],
+    [{"ri": 200}, 200.0],
+    [{"RI": 200}, 200.0],
+    [{"nothing": "200"}, None]
+])
+def test_add_retention_index(metadata, expected):
+    spectrum_in = SpectrumBuilder().with_metadata(metadata).build()
+
+    spectrum = add_retention_index(spectrum_in)
+    actual = spectrum.get("retention_index")
+
+    if expected is None:
+        assert actual is None
+    else:
+        assert actual == expected and isinstance(actual, float)
+
+
+def test_empty_spectrum():
+    spectrum_in = None
+    spectrum = add_retention_time(spectrum_in)
+    assert spectrum is None, "Expected different handling of None spectrum."
+
+    spectrum = add_retention_index(spectrum_in)
+    assert spectrum is None, "Expected different handling of None spectrum."