Repository: matchms/matchms
Pull request #662 (2024-05-27 12:21:45)

fix bug with mz window


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index b4c882ed..defa19c8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,10 +9,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Changed
 - Added remove_profile_spectra filter
 
-### Removed:
-- Require_precursor_below_mz is deprecated, require_precursor_mz now also allows for argument maximum_mz 
-
-
 ## [0.25.0] -2024-05-21
 ### Added
 - filters `require_formula` and `require_compound_name`. [#627](https://github.com/matchms/matchms/pull/627)
@@ -35,7 +31,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Removed repair_precursor_is_parent_mass
 
 - Removed option accept_parent_mass_is_mol_wt in Repair_adduct_based_on_smiles
-- Merged require_precursor_mz and require_precursor_mz_below_mz into require_precursor_mz_below_mz
 - Added repair_adduct_based_on_parent_mass
 - Changed repair_adduct_and_parent_mass_based_on_smiles to update parent mass to the monoisotopic mass of the smiles, instead of updating based on precursor_mz and new adduct. 
 ## [0.24.1] -2024-01-16
diff --git a/matchms/filtering/__init__.py b/matchms/filtering/__init__.py
index 96a8b8e4..e020f368 100644
--- a/matchms/filtering/__init__.py
+++ b/matchms/filtering/__init__.py
@@ -128,8 +128,10 @@ from matchms.filtering.metadata_processing.require_formula import \
     require_formula
 from matchms.filtering.metadata_processing.require_parent_mass_match_smiles import \
     require_parent_mass_match_smiles
-from matchms.filtering.metadata_processing.require_precursor_mz import (
-    require_precursor_below_mz, require_precursor_mz)
+from matchms.filtering.metadata_processing.require_precursor_below_mz import \
+    require_precursor_below_mz
+from matchms.filtering.metadata_processing.require_precursor_mz import \
+    require_precursor_mz
 from matchms.filtering.metadata_processing.require_retention_index import \
     require_retention_index
 from matchms.filtering.metadata_processing.require_retention_time import \
diff --git a/matchms/filtering/default_pipelines.py b/matchms/filtering/default_pipelines.py
index cbaea221..7a84abce 100644
--- a/matchms/filtering/default_pipelines.py
+++ b/matchms/filtering/default_pipelines.py
@@ -1,5 +1,4 @@
 """Stores sets of filters for easy pipeline creation."""
-import matchms.filtering.metadata_processing.require_precursor_mz
 from matchms import filtering as msfilters
 
 
@@ -49,14 +48,14 @@ CLEAN_PEAKS = [(msfilters.select_by_mz, {"mz_from": 0, "mz_to": 1000}),
                msfilters.remove_profiled_spectra,
                ]
 # These filters are in None of the above pipelines
-OTHER_FILTERS = [matchms.filtering.metadata_processing.require_precursor_mz.require_precursor_below_mz,
+OTHER_FILTERS = [msfilters.require_precursor_below_mz,
                  msfilters.select_by_intensity,
                  msfilters.remove_peaks_around_precursor_mz,
                  msfilters.remove_peaks_outside_top_k,
                  msfilters.require_minimum_number_of_peaks,
                  msfilters.add_fingerprint,
                  msfilters.add_losses,
-                 msfilters.repair_parent_mass_match_smiles_wrapper, ]
+                 msfilters.repair_parent_mass_match_smiles_wrapper,]
 
 BASIC_FILTERS = HARMONIZE_METADATA_FIELD_NAMES + DERIVE_METADATA_IN_WRONG_FIELD + HARMONIZE_METADATA_ENTRIES
 DEFAULT_FILTERS = BASIC_FILTERS + [msfilters.normalize_intensities, ] + REQUIRE_COMPLETE_METADATA + DERIVE_MISSING_METADATA
diff --git a/matchms/filtering/metadata_processing/require_precursor_below_mz.py b/matchms/filtering/metadata_processing/require_precursor_below_mz.py
new file mode 100644
index 00000000..12974c94
--- /dev/null
+++ b/matchms/filtering/metadata_processing/require_precursor_below_mz.py
@@ -0,0 +1,37 @@
+import logging
+from matchms.typing import SpectrumType
+
+
+logger = logging.getLogger("matchms")
+
+
+def require_precursor_below_mz(spectrum_in: SpectrumType, max_mz: float = 1000) -> SpectrumType:
+
+    """Returns None if the precursor_mz of a spectrum is above
+       max_mz.
+
+    Parameters
+    ----------
+    spectrum_in:
+        Input spectrum.
+    max_mz:
+        Maximum mz value for the precursor mz of a spectrum.
+        All precursor mz values greater or equal to this
+        will return none. Default is 1000.
+    """
+    if spectrum_in is None:
+        return None
+
+    spectrum = spectrum_in.clone()
+
+    precursor_mz = spectrum.get("precursor_mz", None)
+    assert precursor_mz is not None, "Precursor mz absent."
+    assert isinstance(precursor_mz, (float, int)), ("Expected 'precursor_mz' to be a scalar number.",
+                                                    "Consider applying 'add_precursor_mz' filter first.")
+    assert max_mz >= 0, "max_mz must be a positive scalar."
+    if precursor_mz >= max_mz:
+        logger.info("Spectrum with precursor_mz %s (>%s) was set to None.",
+                    str(precursor_mz), str(max_mz))
+        return None
+
+    return spectrum
diff --git a/matchms/filtering/metadata_processing/require_precursor_mz.py b/matchms/filtering/metadata_processing/require_precursor_mz.py
index 8ce34174..0c134ab3 100644
--- a/matchms/filtering/metadata_processing/require_precursor_mz.py
+++ b/matchms/filtering/metadata_processing/require_precursor_mz.py
@@ -1,5 +1,5 @@
 import logging
-from typing import Optional, Union
+from typing import Union
 from matchms.typing import SpectrumType
 
 
@@ -7,8 +7,7 @@ logger = logging.getLogger("matchms")
 
 
 def require_precursor_mz(spectrum_in: SpectrumType,
-                         minimum_accepted_mz: Optional[float] = 10.0,
-                         maximum_mz: Optional[float] = None
+                         minimum_accepted_mz: float = 10.0
                          ) -> Union[SpectrumType, None]:
 
     """Returns None if there is no precursor_mz or if <= minimum_accepted_mz
@@ -19,8 +18,6 @@ def require_precursor_mz(spectrum_in: SpectrumType,
         Input spectrum.
     minimum_accepted_mz:
         Set to minimum acceptable value for precursor m/z. Default is set to 10.0.
-    maximum_mz:
-        Set the maximum value for precursor m/z.
     """
     if spectrum_in is None:
         return None
@@ -35,35 +32,11 @@ def require_precursor_mz(spectrum_in: SpectrumType,
             "Consider applying 'add_precursor_mz' filter first."
         return None
 
-    if not isinstance(precursor_mz, (float, int)):
-        logger.warning("Precursor mz was not a number (%s) consider applying 'add_precursor_mz' filter first",
-                       precursor_mz)
+    assert isinstance(precursor_mz, (float, int)), \
+        ("Expected 'precursor_mz' to be a scalar number.",
+         "Consider applying 'add_precursor_mz' filter first.")
+    if precursor_mz <= minimum_accepted_mz:
+        logger.info("Spectrum without precursor_mz was set to None.")
         return None
-    if minimum_accepted_mz is not None:
-        if precursor_mz < minimum_accepted_mz:
-            logger.info("Spectrum is removed since precursor mz (%s) was below minimum mz (%s)",
-                        precursor_mz, minimum_accepted_mz)
-            return None
-    if maximum_mz is not None:
-        if precursor_mz > maximum_mz:
-            logger.info("Spectrum is removed since precursor mz (%s) was above maximum mz (%s)",
-                        precursor_mz, maximum_mz)
-            return None
-    return spectrum
-
-
-def require_precursor_below_mz(spectrum_in: SpectrumType, max_mz: float = 1000) -> SpectrumType:
-    """Returns None if the precursor_mz of a spectrum is above
-       max_mz.
 
-    Parameters
-    ----------
-    spectrum_in:
-        Input spectrum.
-    max_mz:
-        Maximum mz value for the precursor mz of a spectrum.
-        All precursor mz values greater or equal to this
-        will return none. Default is 1000.
-    """
-    logger.warning("require_precursor_below_mz is deprecated, please use require_precursor_mz instead")
-    return require_precursor_mz(spectrum_in, minimum_accepted_mz=0, maximum_mz=max_mz)
+    return spectrum
diff --git a/matchms/filtering/peak_processing/remove_profiled_spectra.py b/matchms/filtering/peak_processing/remove_profiled_spectra.py
index 0ca58991..d28ec51d 100644
--- a/matchms/filtering/peak_processing/remove_profiled_spectra.py
+++ b/matchms/filtering/peak_processing/remove_profiled_spectra.py
@@ -45,7 +45,7 @@ def _get_number_of_high_intensity_surounding_peaks(intensities, mz, mz_window):
 
 def _select_intensities_within_mz_window(intensities, mz, mz_span):
     base_peak_i = intensities.argmax()
-    within_mz_window = (mz > mz[base_peak_i] - mz_span) & (mz > mz[base_peak_i] - mz_span)
+    within_mz_window = (mz > mz[base_peak_i] - mz_span) & (mz < mz[base_peak_i] + mz_span)
     intensities_within_mz_window = intensities[within_mz_window]
     return intensities_within_mz_window
 
diff --git a/matchms/importing/load_from_mgf.py b/matchms/importing/load_from_mgf.py
index 52a4bf58..dec6a810 100644
--- a/matchms/importing/load_from_mgf.py
+++ b/matchms/importing/load_from_mgf.py
@@ -1,4 +1,3 @@
-import os
 from typing import Generator, TextIO, Union
 from pyteomics.mgf import MGF
 from matchms.importing.parsing_utils import parse_spectrum_dict
@@ -34,14 +33,8 @@ def load_from_mgf(filename: Union[str, TextIO],
         Set to False if metadata harmonization to default keys is not desired.
         The default is True.
     """
-    if not os.path.isfile(filename):
-        raise FileNotFoundError(f"The specified file: {filename} doesn't exist.") 
-    
-    def parse_file():
-        with MGF(filename, convert_arrays=1) as reader:
-            for pyteomics_spectrum in reader:
-                yield parse_spectrum_dict(
-                    spectrum=pyteomics_spectrum,
-                    metadata_harmonization=metadata_harmonization)
-    
-    return parse_file()
+    with MGF(filename, convert_arrays=1) as reader:
+        for pyteomics_spectrum in reader:
+            yield parse_spectrum_dict(
+                spectrum=pyteomics_spectrum,
+                metadata_harmonization=metadata_harmonization)


Test patch:
diff --git a/tests/filtering/test_require_precursor_below_mz.py b/tests/filtering/test_require_precursor_below_mz.py
index 2b83ecc7..53bcb701 100644
--- a/tests/filtering/test_require_precursor_below_mz.py
+++ b/tests/filtering/test_require_precursor_below_mz.py
@@ -1,7 +1,9 @@
 import numpy as np
 import pytest
+from testfixtures import LogCapture
 from matchms.filtering import require_precursor_below_mz
-from matchms.logging_functions import set_matchms_logger_level
+from matchms.logging_functions import (reset_matchms_logger,
+                                       set_matchms_logger_level)
 from ..builder_Spectrum import SpectrumBuilder
 
 
@@ -26,6 +28,26 @@ def test_if_spectrum_is_cloned():
     assert not spectrum_in.get("testfield"), "Expected input spectrum to remain unchanged."
 
 
+def test_require_precursor_below_without_precursor_mz():
+    """Test if correct assert error is raised for missing precursor-mz."""
+    spectrum_in = SpectrumBuilder().build()
+
+    with pytest.raises(AssertionError) as msg:
+        _ = require_precursor_below_mz(spectrum_in)
+
+    assert str(msg.value) == "Precursor mz absent.", "Expected different error message."
+
+
+def test_require_precursor_below_with_wrong_precursor_mz():
+    """Test if correct assert error is raised for precursor-mz as string."""
+    spectrum_in = SpectrumBuilder().with_metadata({"precursor_mz": "445.0"}).build()
+
+    with pytest.raises(AssertionError) as msg:
+        _ = require_precursor_below_mz(spectrum_in)
+
+    assert "Expected 'precursor_mz' to be a scalar number." in str(msg.value)
+
+
 def test_with_input_none():
     """Test if input spectrum is None."""
     spectrum_in = None
@@ -53,6 +75,11 @@ def test_require_precursor_below_mz_max_50():
     spectrum_in = SpectrumBuilder().with_mz(mz).with_intensities(intensities).build()
     spectrum_in.set("precursor_mz", 60.)
 
-    spectrum = require_precursor_below_mz(spectrum_in, max_mz=50)
+    with LogCapture() as log:
+        spectrum = require_precursor_below_mz(spectrum_in, max_mz=50)
 
     assert spectrum is None, "Expected spectrum to be None."
+    log.check(
+        ('matchms', 'INFO', 'Spectrum with precursor_mz 60.0 (>50) was set to None.')
+    )
+    reset_matchms_logger()
diff --git a/tests/filtering/test_require_precursor_mz.py b/tests/filtering/test_require_precursor_mz.py
index ac7d8e49..8d6c001e 100644
--- a/tests/filtering/test_require_precursor_mz.py
+++ b/tests/filtering/test_require_precursor_mz.py
@@ -1,7 +1,10 @@
 import numpy as np
 import pytest
+from testfixtures import LogCapture
 from matchms.filtering.metadata_processing.require_precursor_mz import \
     require_precursor_mz
+from matchms.logging_functions import (reset_matchms_logger,
+                                       set_matchms_logger_level)
 from ..builder_Spectrum import SpectrumBuilder
 
 
@@ -31,6 +34,16 @@ def test_if_spectrum_is_cloned():
         "Expected input spectrum to remain unchanged."
 
 
+def test_require_precursor_mz_with_wrong_precursor_mz():
+    """Test if correct assert error is raised for precursor-mz as string."""
+    spectrum_in = SpectrumBuilder().with_metadata({"precursor_mz": "445.0"}).build()
+
+    with pytest.raises(AssertionError) as msg:
+        _ = require_precursor_mz(spectrum_in)
+
+    assert "Expected 'precursor_mz' to be a scalar number." in str(msg.value)
+
+
 def test_require_precursor_mz_with_input_none():
     """Test if input spectrum is None."""
     spectrum_in = None
@@ -38,14 +51,23 @@ def test_require_precursor_mz_with_input_none():
     assert spectrum is None
 
 
-@pytest.mark.parametrize("precursor_mz", [0, 9.0, -200])
+@pytest.mark.parametrize("precursor_mz", [0, 10.0, -200])
 def test_require_precursor_mz_fail_when_mz_too_small(precursor_mz):
     """Test if spectrum is None when precursor_mz <= minimum_accepted_mz"""
-    spectrum_in = SpectrumBuilder().build()
+    set_matchms_logger_level("INFO")
+    mz = np.array([10, 20, 30, 40], dtype="float")
+    intensities = np.array([0, 1, 10, 100], dtype="float")
+    spectrum_in = SpectrumBuilder().with_mz(mz).with_intensities(intensities).build()
     spectrum_in.set("precursor_mz", precursor_mz)
 
-    spectrum = require_precursor_mz(spectrum_in, minimum_accepted_mz=10)
+    with LogCapture() as log:
+        spectrum = require_precursor_mz(spectrum_in)
+
     assert spectrum is None, "Expected spectrum to be None."
+    log.check(
+        ('matchms', 'INFO', 'Spectrum without precursor_mz was set to None.')
+    )
+    reset_matchms_logger()
 
 
 def test_require_precursor_mz_fail_because_below_zero():
diff --git a/tests/importing/test_load_from_mgf.py b/tests/importing/test_load_from_mgf.py
index 26596417..1ea22f95 100644
--- a/tests/importing/test_load_from_mgf.py
+++ b/tests/importing/test_load_from_mgf.py
@@ -1,5 +1,4 @@
 import os
-import pytest
 from matchms import Spectrum
 from matchms.importing import load_from_mgf
 
@@ -12,8 +11,3 @@ def test_load_from_mgf_using_filepath():
 
     assert len(spectra) > 0
     assert isinstance(spectra[0], Spectrum)
-
-
-def test_load_missing_mgf_raises():
-    with pytest.raises(FileNotFoundError):
-        load_from_mgf('does-not-exist.mgf')
diff --git a/tests/similarity/test_metadata_match.py b/tests/similarity/test_metadata_match.py
index 8ea53a39..3bdf7d47 100644
--- a/tests/similarity/test_metadata_match.py
+++ b/tests/similarity/test_metadata_match.py
@@ -88,5 +88,7 @@ def test_metadata_match_invalid_array_type(spectrums):
 
     similarity_score = MetadataMatch(field="instrument_type")
 
-    with pytest.raises(ValueError, match="array_type must be 'numpy' or 'sparse'."):
+    try:
         calculate_scores(references, queries, similarity_score, array_type = "scipy")
+    except ValueError as e:
+        assert str(e) == "array_type must be 'numpy' or 'sparse'.", "The error message did not match the expected output"