From 9e03eba13a29365016dd55dcf67ebbb452278205 Mon Sep 17 00:00:00 2001 From: "P. L. Lim" <2090236+pllim@users.noreply.github.com> Date: Fri, 28 Feb 2025 14:02:33 -0500 Subject: [PATCH 1/8] bkg_statistic to take func instead of str Update test_background.py --- specreduce/background.py | 31 ++++++++++------------------- specreduce/tests/test_background.py | 11 ++-------- 2 files changed, 12 insertions(+), 30 deletions(-) diff --git a/specreduce/background.py b/specreduce/background.py index af30b6d..df4f60f 100644 --- a/specreduce/background.py +++ b/specreduce/background.py @@ -317,7 +317,7 @@ def bkg_image(self, image=None): spectral_axis=image.spectral_axis, **kwargs ) - def bkg_spectrum(self, image=None, bkg_statistic="sum"): + def bkg_spectrum(self, image=None, bkg_statistic=np.nansum): """ Expose the 1D spectrum of the background. @@ -328,39 +328,28 @@ def bkg_spectrum(self, image=None, bkg_statistic="sum"): (spatial) direction is axis 0 and dispersion (wavelength) direction is axis 1. If None, will extract the background from ``image`` used to initialize the class. [default: None] - bkg_statistic : {'average', 'median', 'sum'}, optional - Statistical method used to collapse the background image. [default: ``'sum'``] - Supported values are: - - - ``'average'`` : Uses the mean (`numpy.nanmean`). - - ``'median'`` : Uses the median (`numpy.nanmedian`). - - ``'sum'`` : Uses the sum (`numpy.nansum`). + bkg_statistic : func, optional + Statistical method used to collapse the background image. + For historical reason, the default is `numpy.nansum` but + `numpy.nanmedian` or `numpy.nanmean` are better suited for this option. + If you provide your own function, it must take an `~astropy.units.Quantity` + array as input and accept an ``axis`` argument. Returns ------- spec : `~specutils.Spectrum1D` The background 1-D spectrum, with flux expressed in the same - units as the input image (or u.DN if none were provided) and + units as the input image (or DN if none were provided) and the spectral axis expressed in pixel units. """ bkg_image = self.bkg_image(image) - if bkg_statistic == 'sum': - statistic_function = np.nansum - elif bkg_statistic == 'median': - statistic_function = np.nanmedian - elif bkg_statistic == 'average': - statistic_function = np.nanmean - else: - raise ValueError(f"Background statistic {bkg_statistic} is not supported. " - "Please choose from: average, median, or sum.") - try: - return bkg_image.collapse(statistic_function, axis=self.crossdisp_axis) + return bkg_image.collapse(bkg_statistic, axis=self.crossdisp_axis) except u.UnitTypeError: # can't collapse with a spectral axis in pixels because # SpectralCoord only allows frequency/wavelength equivalent units... - ext1d = statistic_function(bkg_image.flux, axis=self.crossdisp_axis) + ext1d = bkg_statistic(bkg_image.flux, axis=self.crossdisp_axis) return Spectrum(ext1d, bkg_image.spectral_axis) def sub_image(self, image=None): diff --git a/specreduce/tests/test_background.py b/specreduce/tests/test_background.py index b54e01e..f4e7d1f 100644 --- a/specreduce/tests/test_background.py +++ b/specreduce/tests/test_background.py @@ -82,19 +82,12 @@ def test_background( assert np.isnan(bg.bkg_spectrum().flux).sum() == 0 assert np.isnan(bg.sub_spectrum().flux).sum() == 0 - bkg_spec_avg = bg1.bkg_spectrum(bkg_statistic="average") + bkg_spec_avg = bg1.bkg_spectrum(bkg_statistic=np.nanmean) assert_allclose(bkg_spec_avg.mean().value, 14.5, rtol=0.5) - bkg_spec_median = bg1.bkg_spectrum(bkg_statistic="median") + bkg_spec_median = bg1.bkg_spectrum(bkg_statistic=np.nanmedian) assert_allclose(bkg_spec_median.mean().value, 14.5, rtol=0.5) - with pytest.raises( - ValueError, - match="Background statistic max is not supported. " - "Please choose from: average, median, or sum.", - ): - bg1.bkg_spectrum(bkg_statistic="max") - def test_warnings_errors(mk_test_spec_no_spectral_axis): image = mk_test_spec_no_spectral_axis From 216d806ab925977c664594b03bea68c2b4fcf898 Mon Sep 17 00:00:00 2001 From: "P. L. Lim" <2090236+pllim@users.noreply.github.com> Date: Mon, 17 Mar 2025 21:23:10 -0400 Subject: [PATCH 2/8] Change default value and add change log --- CHANGES.rst | 7 ++++++- specreduce/background.py | 4 +--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 824a058..ce098dc 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -1,4 +1,4 @@ -1.5.1 (unreleased) +1.5.2 (unreleased) ------------------ New Features @@ -7,8 +7,13 @@ New Features API Changes ^^^^^^^^^^^ +- ``bkg_statistic`` keyword in ``Background.bkg_spectrum()`` now takes callable + instead of string. Its default is also changed from ``"sum"`` (``np.nansum``) + to ``np.nanmedian`` to better suit background calculation. [#253] + Bug Fixes ^^^^^^^^^ + - When all-zero bin encountered in fit_trace with peak_method=gaussian, the bin peak will be set to NaN in this caseto work better with DogBoxLSQFitter. [#257] diff --git a/specreduce/background.py b/specreduce/background.py index df4f60f..cc07152 100644 --- a/specreduce/background.py +++ b/specreduce/background.py @@ -317,7 +317,7 @@ def bkg_image(self, image=None): spectral_axis=image.spectral_axis, **kwargs ) - def bkg_spectrum(self, image=None, bkg_statistic=np.nansum): + def bkg_spectrum(self, image=None, bkg_statistic=np.nanmedian): """ Expose the 1D spectrum of the background. @@ -330,8 +330,6 @@ def bkg_spectrum(self, image=None, bkg_statistic=np.nansum): from ``image`` used to initialize the class. [default: None] bkg_statistic : func, optional Statistical method used to collapse the background image. - For historical reason, the default is `numpy.nansum` but - `numpy.nanmedian` or `numpy.nanmean` are better suited for this option. If you provide your own function, it must take an `~astropy.units.Quantity` array as input and accept an ``axis`` argument. From fa0a3d5d4c98a48e62a23d4892c1c523ae4794a8 Mon Sep 17 00:00:00 2001 From: "P. L. Lim" <2090236+pllim@users.noreply.github.com> Date: Mon, 17 Mar 2025 21:27:12 -0400 Subject: [PATCH 3/8] Fix tests --- specreduce/tests/test_background.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/specreduce/tests/test_background.py b/specreduce/tests/test_background.py index f4e7d1f..c33e19d 100644 --- a/specreduce/tests/test_background.py +++ b/specreduce/tests/test_background.py @@ -59,7 +59,7 @@ def test_background( assert np.allclose(sub4.flux, sub5.flux) assert np.allclose(sub5.flux, sub6.flux) - bkg_spec = bg1.bkg_spectrum() + bkg_spec = bg1.bkg_spectrum(bkg_statistic=np.nansum) assert isinstance(bkg_spec, Spectrum) sub_spec = bg1.sub_spectrum() assert isinstance(sub_spec, Spectrum) @@ -79,7 +79,7 @@ def test_background( bg = Background(img, trace - bkg_sep, width=bkg_width, statistic=st) assert np.isnan(bg.image.flux).sum() == 2 assert np.isnan(bg._bkg_array).sum() == 0 - assert np.isnan(bg.bkg_spectrum().flux).sum() == 0 + assert np.isnan(bg.bkg_spectrum(bkg_statistic=np.nansum).flux).sum() == 0 assert np.isnan(bg.sub_spectrum().flux).sum() == 0 bkg_spec_avg = bg1.bkg_spectrum(bkg_statistic=np.nanmean) @@ -288,7 +288,7 @@ def test_mask_treatment_bkg_img_spectrum(self, method, expected): # test background spectrum matches 'expected' times the number of rows # in cross disp axis, since this is a sum and all values in a col are # the same. - bk_spec = background.bkg_spectrum() + bk_spec = background.bkg_spectrum(bkg_statistic=np.nansum) np.testing.assert_allclose(bk_spec.flux.value, expected * img_size) def test_sub_bkg_image(self): From 184d3b94a2212209e9729a20fa66a9fae79fa63b Mon Sep 17 00:00:00 2001 From: Hannu Parviainen Date: Fri, 28 Mar 2025 07:28:04 +0000 Subject: [PATCH 4/8] Moved the "statistic" argument in background estimation to __post_init__ from bgk_spectrum. --- specreduce/background.py | 50 +++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 29 deletions(-) diff --git a/specreduce/background.py b/specreduce/background.py index cc07152..7ad3197 100644 --- a/specreduce/background.py +++ b/specreduce/background.py @@ -1,6 +1,7 @@ # Licensed under a 3-clause BSD style license - see LICENSE.rst import warnings +from collections.abc import Callable from dataclasses import dataclass, field import numpy as np @@ -39,7 +40,8 @@ class Background(_ImageParser): statistic Statistic to use when computing the background. ``average`` will account for partial pixel weights, ``median`` will include all partial - pixels. + pixels. If you provide your own function, it must take a `~numpy.ma.MaskedArray` + masked array as input and accept an ``axis`` argument. disp_axis Dispersion axis. crossdisp_axis @@ -70,7 +72,7 @@ class Background(_ImageParser): image: ImageLike traces: list = field(default_factory=list) width: float = 5 - statistic: str = "average" + statistic: str | Callable[..., np.ndarray] = "average" disp_axis: int = 1 crossdisp_axis: int = 0 mask_treatment: MaskingOption = "apply" @@ -146,13 +148,13 @@ def __post_init__(self): self.bkg_wimage = bkg_wimage - if self.statistic == "average": + if isinstance(self.statistic, Callable): + img.mask = (self.bkg_wimage == 0) | self.image.mask + self._bkg_array = self.statistic(img, axis=self.crossdisp_axis) + elif self.statistic == "average": self._bkg_array = np.ma.average(img, weights=self.bkg_wimage, axis=self.crossdisp_axis) - elif self.statistic == "median": - # combine where background weight image is 0 with image masked (which already - # accounts for non-finite data that wasn't already masked) - img.mask = np.logical_or(self.bkg_wimage == 0, self.image.mask) + img.mask = (self.bkg_wimage == 0) | self.image.mask self._bkg_array = np.ma.median(img, axis=self.crossdisp_axis) else: raise ValueError("statistic must be 'average' or 'median'") @@ -216,10 +218,11 @@ def two_sided(cls, image, trace_object, separation, **kwargs): separation from ``trace_object`` for the background regions width : float width of each background aperture in pixels - statistic: string - statistic to use when computing the background. 'average' will - account for partial pixel weights, 'median' will include all partial - pixels. + statistic: string or Callable + Statistic to use when computing the background. ``average`` will + account for partial pixel weights, ``median`` will include all partial + pixels. If you provide your own function, it must take a `~numpy.ma.MaskedArray` + masked array as input and accept an ``axis`` argument. disp_axis : int dispersion axis crossdisp_axis : int @@ -265,10 +268,11 @@ def one_sided(cls, image, trace_object, separation, **kwargs): above the trace, negative below. width : float width of each background aperture in pixels - statistic: string - statistic to use when computing the background. 'average' will - account for partial pixel weights, 'median' will include all partial - pixels. + statistic: string or Callable + Statistic to use when computing the background. ``average`` will + account for partial pixel weights, ``median`` will include all partial + pixels. If you provide your own function, it must take a `~numpy.ma.MaskedArray` + masked array as input and accept an ``axis`` argument. disp_axis : int dispersion axis crossdisp_axis : int @@ -317,7 +321,7 @@ def bkg_image(self, image=None): spectral_axis=image.spectral_axis, **kwargs ) - def bkg_spectrum(self, image=None, bkg_statistic=np.nanmedian): + def bkg_spectrum(self, image=None): """ Expose the 1D spectrum of the background. @@ -328,10 +332,6 @@ def bkg_spectrum(self, image=None, bkg_statistic=np.nanmedian): (spatial) direction is axis 0 and dispersion (wavelength) direction is axis 1. If None, will extract the background from ``image`` used to initialize the class. [default: None] - bkg_statistic : func, optional - Statistical method used to collapse the background image. - If you provide your own function, it must take an `~astropy.units.Quantity` - array as input and accept an ``axis`` argument. Returns ------- @@ -340,15 +340,7 @@ def bkg_spectrum(self, image=None, bkg_statistic=np.nanmedian): units as the input image (or DN if none were provided) and the spectral axis expressed in pixel units. """ - bkg_image = self.bkg_image(image) - - try: - return bkg_image.collapse(bkg_statistic, axis=self.crossdisp_axis) - except u.UnitTypeError: - # can't collapse with a spectral axis in pixels because - # SpectralCoord only allows frequency/wavelength equivalent units... - ext1d = bkg_statistic(bkg_image.flux, axis=self.crossdisp_axis) - return Spectrum(ext1d, bkg_image.spectral_axis) + return Spectrum(self._bkg_array) def sub_image(self, image=None): """ From d9b8a52fba61bfd0e1c5d2e15c3bcafff3471375 Mon Sep 17 00:00:00 2001 From: Hannu Parviainen Date: Mon, 7 Apr 2025 14:37:13 +0100 Subject: [PATCH 5/8] Removed the use of 'bkg_statistic' from Background tests and fixed Background.bkg_image Spectrum initialisation. --- specreduce/background.py | 14 +++++++------- specreduce/tests/test_background.py | 21 ++++++++------------- 2 files changed, 15 insertions(+), 20 deletions(-) diff --git a/specreduce/background.py b/specreduce/background.py index 7ad3197..a9fac81 100644 --- a/specreduce/background.py +++ b/specreduce/background.py @@ -157,7 +157,10 @@ def __post_init__(self): img.mask = (self.bkg_wimage == 0) | self.image.mask self._bkg_array = np.ma.median(img, axis=self.crossdisp_axis) else: - raise ValueError("statistic must be 'average' or 'median'") + raise ValueError( + "statistic must be 'average', 'median', or a callable function that takes a masked" + "array as input and an axis argument." + ) def _set_traces(self): """Determine `traces` from input. If an integer/float or list if int/float @@ -316,10 +319,7 @@ def bkg_image(self, image=None): kwargs = {} else: kwargs = {"spectral_axis_index": arr.ndim - 1} - return Spectrum( - arr * image.unit, - spectral_axis=image.spectral_axis, **kwargs - ) + return Spectrum(arr * image.unit, spectral_axis=image.spectral_axis, **kwargs) def bkg_spectrum(self, image=None): """ @@ -340,7 +340,7 @@ def bkg_spectrum(self, image=None): units as the input image (or DN if none were provided) and the spectral axis expressed in pixel units. """ - return Spectrum(self._bkg_array) + return Spectrum(self._bkg_array * self.image.unit, spectral_axis=self.image.spectral_axis) def sub_image(self, image=None): """ @@ -349,7 +349,7 @@ def sub_image(self, image=None): Parameters ---------- image : nddata-compatible image or None - image with 2-D spectral image data. If None, will extract + image with 2-D spectral image data. If None, will subtract the background from ``image`` used to initialize the class. Returns diff --git a/specreduce/tests/test_background.py b/specreduce/tests/test_background.py index c33e19d..afb6c56 100644 --- a/specreduce/tests/test_background.py +++ b/specreduce/tests/test_background.py @@ -59,7 +59,7 @@ def test_background( assert np.allclose(sub4.flux, sub5.flux) assert np.allclose(sub5.flux, sub6.flux) - bkg_spec = bg1.bkg_spectrum(bkg_statistic=np.nansum) + bkg_spec = bg1.bkg_spectrum() assert isinstance(bkg_spec, Spectrum) sub_spec = bg1.sub_spectrum() assert isinstance(sub_spec, Spectrum) @@ -73,19 +73,18 @@ def test_background( # the final 1D spectra. img[0, 0] = np.nan # out of window img[trace_pos, 0] = np.nan # in window - stats = ["average", "median"] + stats = ["average", "median", np.nanmean, np.nanmedian] for st in stats: bg = Background(img, trace - bkg_sep, width=bkg_width, statistic=st) assert np.isnan(bg.image.flux).sum() == 2 assert np.isnan(bg._bkg_array).sum() == 0 - assert np.isnan(bg.bkg_spectrum(bkg_statistic=np.nansum).flux).sum() == 0 - assert np.isnan(bg.sub_spectrum().flux).sum() == 0 + assert np.isnan(bg.bkg_spectrum().flux).sum() == 0 - bkg_spec_avg = bg1.bkg_spectrum(bkg_statistic=np.nanmean) + bkg_spec_avg = bg1.bkg_spectrum() assert_allclose(bkg_spec_avg.mean().value, 14.5, rtol=0.5) - bkg_spec_median = bg1.bkg_spectrum(bkg_statistic=np.nanmedian) + bkg_spec_median = bg1.bkg_spectrum() assert_allclose(bkg_spec_median.mean().value, 14.5, rtol=0.5) @@ -281,15 +280,11 @@ def test_mask_treatment_bkg_img_spectrum(self, method, expected): # test background image matches 'expected' bk_img = background.bkg_image() - # change this and following assertions to assert_quantity_allclose once - # issue #213 is fixed np.testing.assert_allclose(bk_img.flux.value, np.tile(expected, (img_size, 1))) - # test background spectrum matches 'expected' times the number of rows - # in cross disp axis, since this is a sum and all values in a col are - # the same. - bk_spec = background.bkg_spectrum(bkg_statistic=np.nansum) - np.testing.assert_allclose(bk_spec.flux.value, expected * img_size) + # test background spectrum matches 'expected' + bk_spec = background.bkg_spectrum() + np.testing.assert_allclose(bk_spec.flux.value, expected) def test_sub_bkg_image(self): """ From edd9366e658cb7ec4f9bdfbf8f53e62486e4577e Mon Sep 17 00:00:00 2001 From: Hannu Parviainen Date: Mon, 7 Apr 2025 14:45:30 +0100 Subject: [PATCH 6/8] Added a deprecation warning to the `bkg_statistic` argument in `bkg_spectrum`, advising users to use the `statistic` argument in the Background initializer instead. --- specreduce/background.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/specreduce/background.py b/specreduce/background.py index a9fac81..983f5ca 100644 --- a/specreduce/background.py +++ b/specreduce/background.py @@ -321,7 +321,7 @@ def bkg_image(self, image=None): kwargs = {"spectral_axis_index": arr.ndim - 1} return Spectrum(arr * image.unit, spectral_axis=image.spectral_axis, **kwargs) - def bkg_spectrum(self, image=None): + def bkg_spectrum(self, image=None, bkg_statistic=None): """ Expose the 1D spectrum of the background. @@ -340,6 +340,12 @@ def bkg_spectrum(self, image=None): units as the input image (or DN if none were provided) and the spectral axis expressed in pixel units. """ + if bkg_statistic is not None: + warnings.warn( + "'bkg_statistic' is deprecated and will be removed in a future release. " + "Please use the 'statistic' argument in the Background initializer instead.", + DeprecationWarning, + ) return Spectrum(self._bkg_array * self.image.unit, spectral_axis=self.image.spectral_axis) def sub_image(self, image=None): From dbdf9ebc7bd8b528985ea2e9babcfb724696311d Mon Sep 17 00:00:00 2001 From: Hannu Parviainen Date: Mon, 7 Apr 2025 19:34:50 +0100 Subject: [PATCH 7/8] Updated ``Background`` initializer type hints and revised the changelog. --- CHANGES.rst | 9 ++++++--- specreduce/background.py | 3 ++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index ce098dc..96c9945 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -7,9 +7,12 @@ New Features API Changes ^^^^^^^^^^^ -- ``bkg_statistic`` keyword in ``Background.bkg_spectrum()`` now takes callable - instead of string. Its default is also changed from ``"sum"`` (``np.nansum``) - to ``np.nanmedian`` to better suit background calculation. [#253] +- ``statistic`` argument in ``Background`` initializer can now be either ``"average"``, + ``"median"``, or a custom callable that takes a ``numpy.ma.MaskedArray`` masked array + as an input and accepts ``axis`` as an argument. [#253] +- ``bkg_statistic`` keyword in ``Background.bkg_spectrum()`` now deprecated and raises + a warning if set. The ``statistic`` argument in the ``Background`` initializer should + be used instead. [#253] Bug Fixes ^^^^^^^^^ diff --git a/specreduce/background.py b/specreduce/background.py index 983f5ca..0ae4307 100644 --- a/specreduce/background.py +++ b/specreduce/background.py @@ -3,6 +3,7 @@ import warnings from collections.abc import Callable from dataclasses import dataclass, field +from typing import Literal import numpy as np from astropy import units as u @@ -72,7 +73,7 @@ class Background(_ImageParser): image: ImageLike traces: list = field(default_factory=list) width: float = 5 - statistic: str | Callable[..., np.ndarray] = "average" + statistic: Literal["average", "median"] | Callable[..., np.ndarray] = "average" disp_axis: int = 1 crossdisp_axis: int = 0 mask_treatment: MaskingOption = "apply" From 20ff84340d0d43fe18b653e12ae162d95c250aec Mon Sep 17 00:00:00 2001 From: Hannu Parviainen Date: Mon, 7 Apr 2025 20:05:03 +0100 Subject: [PATCH 8/8] Fixed a minor typo in the changelog. --- CHANGES.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.rst b/CHANGES.rst index 96c9945..9cf4e97 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -10,7 +10,7 @@ API Changes - ``statistic`` argument in ``Background`` initializer can now be either ``"average"``, ``"median"``, or a custom callable that takes a ``numpy.ma.MaskedArray`` masked array as an input and accepts ``axis`` as an argument. [#253] -- ``bkg_statistic`` keyword in ``Background.bkg_spectrum()`` now deprecated and raises +- ``bkg_statistic`` keyword in ``Background.bkg_spectrum()`` is now deprecated and raises a warning if set. The ``statistic`` argument in the ``Background`` initializer should be used instead. [#253]