Repository: matchms/matchms

Issue:
Metadata processing of some msp files fails fatally
**Describe the bug**
When using some MoNA MassBank msp files, the metadata extraction fails on line 157 of load_from_msp.py. 

**To Reproduce**
Attempt to load the MSP file here: https://mona.fiehnlab.ucdavis.edu/downloads, specifically the LC-MS/MS positive mode dataset. 

**Expected behavior**
This is probably due to one or more files not complying with the metadata format specified or expected by the parser. This in and of itself is not a problem since non-compliant files are not matchms problem per se; however, I would expect the inability to parse a metadata field to be handled more gracefully. Currently when this issue occurs, the entire extraction fails. On my system I have worked around this by modifying the function and wrapping lines 155-157 in a try catch. 

**Desktop (please complete the following information):**
 - OS: MacOS
 - Browser: N/A
 - Version 0.22.0

**Additional context**
I'd be happy to submit a pull request or to make some tweaks to the metadata parsing to make it more robust to these sorts of issues. I am only working with MoNA currently so I'm not sure how widespread the problem could be. I do have metadata harmonization disabled in the call to load_from_msp. 


Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5153e9d4..234bc96c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Added a save spectra function. To automatically save in the specified file format. [#543](https://github.com/matchms/matchms/pull/543)
 - Add saving function in SpectrumProcessor [#543](https://github.com/matchms/matchms/pull/543)
 ### Fixed
+- Fixed bug when loading empty metadata in msp [#548](https://github.com/matchms/matchms/issues/548)
 - 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)
 - Fix closing mgf file after loading and prevent reopening. [#555](https://github.com/matchms/matchms/issues/555)
 
diff --git a/matchms/importing/load_from_msp.py b/matchms/importing/load_from_msp.py
index 46608584..b2593100 100644
--- a/matchms/importing/load_from_msp.py
+++ b/matchms/importing/load_from_msp.py
@@ -82,7 +82,7 @@ def parse_msp_file(filename: str) -> Generator[dict, None, None]:
 
             masses = np.append(masses, mz)
             intensities = np.append(intensities, ints)
-            
+
             if comment is not None:
                 peak_comments.update({masses[-1]: comment})
 
@@ -103,8 +103,6 @@ def parse_msp_file(filename: str) -> Generator[dict, None, None]:
                 masses = []
                 intensities = []
                 peak_comments = {}
-            
-
 
 
 def _parse_line_with_peaks(rline: str) -> Tuple[List[float], List[float], str]:
@@ -116,9 +114,9 @@ def _parse_line_with_peaks(rline: str) -> Tuple[List[float], List[float], str]:
     Returns:
         Tuple[List[float], List[float], str]: mz, intensity and peak comments obtained from the line.
     """
-    comment, rline = get_peak_comment(rline)   
+    comment, rline = get_peak_comment(rline)
     mz, intensities = get_peak_values(rline)
-    
+
     return mz, intensities, comment
 
 
@@ -127,7 +125,7 @@ def get_peak_values(peak: str) -> Tuple[List[float], List[float]]:
     tokens = re.findall(r'(\d+(?:\.\d+)?(?:e[-+]?\d+)?)', peak)
     if len(tokens) % 2 != 0:
         raise RuntimeError("Wrong peak format detected!")
-    
+
     tokens = list(map(float, tokens))
     mz = tokens[0::2]
     intensities = tokens[1::2]
@@ -145,20 +143,39 @@ def get_peak_comment(rline: str) -> Tuple[str, str]:
 
 
 def parse_metadata(rline: str, params: dict):
-    """ Reads metadata contained in line into params dict. """
+    """ Reads metadata contained in line into params dict.
+
+    The complexity of this function stems from the fact that MSP allows for many different formats of metadata.
+    """
     matches = []
     splitted_line = rline.split(":", 1)
     if splitted_line[0].lower() == 'comments' and "=" in splitted_line[1]:
-        pattern = r'(\S+)="([^"]*)"|"(\w+)=([^"]*)"|"([^"]*)=([^"]*)"|(\S+)=(\d+(?:\.\d*)?)'
+        # This pattern check for different formats.
+        # The first checks for compound="caffeine", the second and third "compound=caffeine and the last for parent_mass=12.1
+        # The second and third almost match the same pattern, but the second is there to ensure tha the pattern
+        # "SMILES=CC(O)C(O)=O" is recognized as 'smiles': 'CC(O)C(O)=O' instead of 'smiles=cc(o)c(o)': 'O'
+        # The third pattern is needed since some keys like DB# should be stored, which is not recognized by the \w regex
+        # Cases like "Smiles=" will not be stores (since len(match) = 1) The reason that we did not change the * to +
+        # in the regex, is that we want to still match to these cases, so that the if len(matches) == 0: below is not
+        # called in cases like comments: "smiles=".
+        pattern = r'(\S+)="([^"]*)"|' \
+                  r'"(\w+)=([^"]*)"|' \
+                  r'"([^"]*)=([^"]*)"|' \
+                  r'(\S+)=(\d+(?:\.\d*)?)'
         matches = re.findall(pattern, splitted_line[1].replace("'", '"'))
         for match in matches:
+            # Remove the None parts
             match = [i for i in match if i]
-            key = match[0]
-            value = match[1]
-            if key.lower().strip() in params.keys() and key.lower().strip() == 'smiles':
-                params[key.lower()+"_2"] = value.strip()
-            else:
-                params[key.lower().strip()] = value.strip()
+            # If len is not 2 a pattern was matched like "compound_name="
+            if len(match) == 2:
+                key = match[0]
+                value = match[1]
+                if key.lower().strip() in params.keys() and key.lower().strip() == 'smiles':
+                    params[key.lower()+"_2"] = value.strip()
+                else:
+                    params[key.lower().strip()] = value.strip()
+    # msp files can have the format comments: smiles="CC=O" but also the format smiles: CC=O.
+    # The latter is captured by these lines.
     if len(matches) == 0:
         params[splitted_line[0].lower()] = splitted_line[1].strip()
 


Test patch:
diff --git a/tests/importing/test_load_from_msp.py b/tests/importing/test_load_from_msp.py
index 562cb4d9..bf46edfc 100644
--- a/tests/importing/test_load_from_msp.py
+++ b/tests/importing/test_load_from_msp.py
@@ -1,7 +1,8 @@
 import os
 import numpy as np
+import pytest
 from matchms import Spectrum
-from matchms.importing import load_from_msp
+from matchms.importing.load_from_msp import load_from_msp, parse_metadata
 from tests.builder_Spectrum import SpectrumBuilder
 
 
@@ -262,5 +263,20 @@ def test_load_msp_with_scientific_notation():
     module_root = os.path.join(os.path.dirname(__file__), "..")
     spectrums_file = os.path.join(module_root, "testdata", "test_spectra_collection.msp")
     actual = list(load_from_msp(spectrums_file))
-    
+
     assert len(actual) == 3
+
+
+@pytest.mark.parametrize("input_line, expected_output", [
+    ['comments: "SMILES="', {}],
+    ['comments: SMILES="CC(O)C(O)=O"', {"smiles": "CC(O)C(O)=O"}],
+    ['comments: mass=12.0', {"mass": '12.0'}],
+    ['name: 3,4-DICHLOROPHENOL', {'name': '3,4-DICHLOROPHENOL'}],
+    ['comments: "SMILES=CC(O)C(O)=O"', {"smiles": "CC(O)C(O)=O"}],
+    ['comments: "DB#=JP000001"', {"db#":"JP000001"}],
+])
+def test_parse_metadata(input_line, expected_output):
+    """tests if metadata is correctly parsed"""
+    params = {}
+    parse_metadata(input_line, params)
+    assert params == expected_output