Repository: matchms/matchms
Pull request #151 (2020-10-14 18:27:39)

Make functions fit for use of GCMS data
- raise exception when ``reduce_to_number_of_peaks`` is used with ``desired_ratio`` but without parent mass (e.g. for GC MS data). Before it would then just ignore the parameter, but that can be very unexpected behavior.
- fix small issue with msp importer function

jspaaks: thanks for the review!

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 26770a42..2b131584 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- Raise error for improper use of reduce_to_number_of_peaks filter [#151](https://github.com/matchms/matchms/pull/151)
+
+### Fixed
+
+- Fix minor issue with msp importer to avoid failing with unknown characters [#151](https://github.com/matchms/matchms/pull/151)
+
 ## [0.6.0] - 2020-09-14
 
 ### Added
diff --git a/matchms/filtering/reduce_to_number_of_peaks.py b/matchms/filtering/reduce_to_number_of_peaks.py
index 4d540c30..f93e5870 100644
--- a/matchms/filtering/reduce_to_number_of_peaks.py
+++ b/matchms/filtering/reduce_to_number_of_peaks.py
@@ -19,6 +19,8 @@ def reduce_to_number_of_peaks(spectrum_in: SpectrumType, n_required: int = 1, n_
         Maximum number of peaks. Remove peaks if more peaks are found.
     ratio_desired:
         Set desired ratio between maximum number of peaks and parent mass.
+        For spectra without parent mass (e.g. GCMS spectra) this will raise an
+        error when ratio_desired is used.
         Default is None.
     """
     def _set_maximum_number_of_peaks_to_keep():
@@ -26,7 +28,9 @@ def reduce_to_number_of_peaks(spectrum_in: SpectrumType, n_required: int = 1, n_
         if parent_mass and ratio_desired:
             n_desired_by_mass = int(ceil(ratio_desired * parent_mass))
             return max(n_required, n_desired_by_mass)
-        return n_max
+        if not ratio_desired:
+            return n_max
+        raise ValueError("Cannot use ratio_desired for spectrum without parent_mass.")
 
     def _remove_lowest_intensity_peaks():
         mz, intensities = spectrum.peaks
diff --git a/matchms/importing/load_from_msp.py b/matchms/importing/load_from_msp.py
index 1f59e89f..400e070e 100644
--- a/matchms/importing/load_from_msp.py
+++ b/matchms/importing/load_from_msp.py
@@ -14,7 +14,7 @@ def parse_msp_file(filename: str) -> Generator[dict, None, None]:
     # Peaks counter. Used to track and count the number of peaks
     peakscount = 0
 
-    with open(filename, 'r') as f:
+    with open(filename, 'r', encoding='utf-8') as f:
         for line in f:
             rline = line.rstrip()
 


Test patch:
diff --git a/tests/test_reduce_to_number_of_peaks.py b/tests/test_reduce_to_number_of_peaks.py
index 2648a4d3..2cf00ed0 100644
--- a/tests/test_reduce_to_number_of_peaks.py
+++ b/tests/test_reduce_to_number_of_peaks.py
@@ -1,10 +1,11 @@
 import numpy
+import pytest
 from matchms import Spectrum
 from matchms.filtering import reduce_to_number_of_peaks
 
 
 def test_reduce_to_number_of_peaks_no_params():
-
+    """Use default parameters."""
     mz = numpy.array([10, 20, 30, 40], dtype="float")
     intensities = numpy.array([0, 1, 10, 100], dtype="float")
     spectrum_in = Spectrum(mz=mz, intensities=intensities)
@@ -14,8 +15,20 @@ def test_reduce_to_number_of_peaks_no_params():
     assert spectrum == spectrum_in, "Expected no changes."
 
 
-def test_reduce_to_number_of_peaks_n_max_4():
+def test_reduce_to_number_of_peaks_no_params_w_parent_mass():
+    """Use default parameters with present parent mass."""
+    mz = numpy.array([10, 20, 30, 40], dtype="float")
+    intensities = numpy.array([0, 1, 10, 100], dtype="float")
+    spectrum_in = Spectrum(mz=mz, intensities=intensities,
+                           metadata={"parent_mass": 50})
+
+    spectrum = reduce_to_number_of_peaks(spectrum_in)
 
+    assert spectrum == spectrum_in, "Expected no changes."
+
+
+def test_reduce_to_number_of_peaks_n_max_4():
+    """Test setting n_max parameter."""
     mz = numpy.array([10, 20, 30, 40, 50], dtype="float")
     intensities = numpy.array([1, 1, 10, 20, 100], dtype="float")
     spectrum_in = Spectrum(mz=mz, intensities=intensities)
@@ -27,14 +40,16 @@ def test_reduce_to_number_of_peaks_n_max_4():
 
 
 def test_reduce_to_number_of_peaks_ratio_given_but_no_parent_mass():
-    """A ratio_desired given without parent_mass should not result in changes."""
+    """A ratio_desired given without parent_mass should raise an exception."""
     mz = numpy.array([10, 20, 30, 40], dtype="float")
     intensities = numpy.array([0, 1, 10, 100], dtype="float")
     spectrum_in = Spectrum(mz=mz, intensities=intensities)
 
-    spectrum = reduce_to_number_of_peaks(spectrum_in, n_required=4, ratio_desired=0.1)
+    with pytest.raises(Exception) as msg:
+        _ = reduce_to_number_of_peaks(spectrum_in, n_required=4, ratio_desired=0.1)
 
-    assert spectrum == spectrum_in, "Expected the spectrum to remain unchanged."
+    expected_msg = "Cannot use ratio_desired for spectrum without parent_mass."
+    assert expected_msg in str(msg.value), "Expected specific exception message."
 
 
 def test_reduce_to_number_of_peaks_required_2_desired_2():
@@ -67,7 +82,8 @@ def test_reduce_to_number_of_peaks_desired_5_check_sorting():
     """Check if mz and intensities order is sorted correctly """
     mz = numpy.array([10, 20, 30, 40, 50, 60], dtype="float")
     intensities = numpy.array([5, 1, 4, 3, 100, 2], dtype="float")
-    spectrum_in = Spectrum(mz=mz, intensities=intensities)
+    spectrum_in = Spectrum(mz=mz, intensities=intensities,
+                           metadata={"parent_mass": 20})
 
     spectrum = reduce_to_number_of_peaks(spectrum_in, n_max=5)