Possibly reduce duplication between `signal` and `ndimage` filtering routines
Hi, In `scipy.signal` and `scipy.ndimage`, we ship a set of almost-if-not-completely overlapping functionality. For instance, - `signal.order_filter` has a counterpart, `ndimage.rank_filter`; - `signal.median_filter` can be implemented in terms of the rank filter In fact, applying the patch below (the patch is based on the CuPy implementation of `cupyx.signal`, see https://github.com/cupy/cupy/blob/v11.5.0/cupyx/scipy/signal/_signaltools.py...), and running `$ python dev.py test -s signal`, I only get three failures in `medfilt`, related to - object arrays of `Decimal` instances - `longdouble` arrays. So, assuming we can deprecate longdoubles and decimals in these filters, we could potentially remove some 700 lines of obscure C code in https://github.com/scipy/scipy/blob/main/scipy/signal/_lfilter.c.in Thoughts? Evgeni P.S. The patch to check the mapping between signal.{order,median}_filter and ndimage.rank_filter: ```diff diff --git a/scipy/signal/_signaltools.py b/scipy/signal/_signaltools.py index 31baba2421..2f00ead3a2 100644 --- a/scipy/signal/_signaltools.py +++ b/scipy/signal/_signaltools.py @@ -1492,7 +1492,15 @@ def order_filter(a, domain, rank): if (dimsize % 2) != 1: raise ValueError("Each dimension of domain argument " "should have an odd number of elements.") - return _sigtools._order_filterND(a, domain, rank) + + result = _sigtools._order_filterND(a, domain, rank) + + import scipy.ndimage as ndimage + import numpy.testing as testing + res_ndi = ndimage.rank_filter(a, rank, footprint=domain, mode='constant') + testing.assert_equal(result, res_ndi) + + return result def medfilt(volume, kernel_size=None): @@ -1555,7 +1563,17 @@ def medfilt(volume, kernel_size=None): numels = np.prod(kernel_size, axis=0) order = numels // 2 - return _sigtools._order_filterND(volume, domain, order) + result = _sigtools._order_filterND(volume, domain, order) + + + import scipy.ndimage as ndimage + import numpy.testing as testing + size = np.prod(kernel_size) + res_ndi = ndimage.rank_filter(volume, size // 2, size=kernel_size, + mode='constant') + testing.assert_equal(result, res_ndi) + + return result def wiener(im, mysize=None, noise=None): ```
On Tue, Feb 21, 2023 at 8:12 PM Evgeni Burovski <evgeny.burovskiy@gmail.com> wrote:
Hi,
In `scipy.signal` and `scipy.ndimage`, we ship a set of almost-if-not-completely overlapping functionality. For instance, - `signal.order_filter` has a counterpart, `ndimage.rank_filter`; - `signal.median_filter` can be implemented in terms of the rank filter
In fact, applying the patch below (the patch is based on the CuPy implementation of `cupyx.signal`, see
https://github.com/cupy/cupy/blob/v11.5.0/cupyx/scipy/signal/_signaltools.py... ), and running `$ python dev.py test -s signal`, I only get three failures in `medfilt`, related to - object arrays of `Decimal` instances - `longdouble` arrays.
So, assuming we can deprecate longdoubles and decimals in these filters, we could potentially remove some 700 lines of obscure C code in https://github.com/scipy/scipy/blob/main/scipy/signal/_lfilter.c.in
Thoughts?
+1 this sounds like a healthy idea. There's a lot of duplication in these types of routines, and it's quite under-maintained code. And these deprecations sound perfectly fine. Cheers, Ralf
Evgeni
P.S. The patch to check the mapping between signal.{order,median}_filter and ndimage.rank_filter:
```diff diff --git a/scipy/signal/_signaltools.py b/scipy/signal/_signaltools.py index 31baba2421..2f00ead3a2 100644 --- a/scipy/signal/_signaltools.py +++ b/scipy/signal/_signaltools.py @@ -1492,7 +1492,15 @@ def order_filter(a, domain, rank): if (dimsize % 2) != 1: raise ValueError("Each dimension of domain argument " "should have an odd number of elements.") - return _sigtools._order_filterND(a, domain, rank) + + result = _sigtools._order_filterND(a, domain, rank) + + import scipy.ndimage as ndimage + import numpy.testing as testing + res_ndi = ndimage.rank_filter(a, rank, footprint=domain, mode='constant') + testing.assert_equal(result, res_ndi) + + return result
def medfilt(volume, kernel_size=None): @@ -1555,7 +1563,17 @@ def medfilt(volume, kernel_size=None):
numels = np.prod(kernel_size, axis=0) order = numels // 2 - return _sigtools._order_filterND(volume, domain, order) + result = _sigtools._order_filterND(volume, domain, order) + + + import scipy.ndimage as ndimage + import numpy.testing as testing + size = np.prod(kernel_size) + res_ndi = ndimage.rank_filter(volume, size // 2, size=kernel_size, + mode='constant') + testing.assert_equal(result, res_ndi) + + return result
def wiener(im, mysize=None, noise=None): ``` _______________________________________________ SciPy-Dev mailing list -- scipy-dev@python.org To unsubscribe send an email to scipy-dev-leave@python.org https://mail.python.org/mailman3/lists/scipy-dev.python.org/ Member address: ralf.gommers@googlemail.com
Just to close the loop, here's a PR: https://github.com/scipy/scipy/pull/18341 Like it says in the PR, I am actually not 100% positive our test suite is exhaustive, so if someone sees a problem with this, please respond here or on the PR. Evgeni On Wed, Feb 22, 2023 at 2:42 PM Ralf Gommers <ralf.gommers@gmail.com> wrote:
On Tue, Feb 21, 2023 at 8:12 PM Evgeni Burovski <evgeny.burovskiy@gmail.com> wrote:
Hi,
In `scipy.signal` and `scipy.ndimage`, we ship a set of almost-if-not-completely overlapping functionality. For instance, - `signal.order_filter` has a counterpart, `ndimage.rank_filter`; - `signal.median_filter` can be implemented in terms of the rank filter
In fact, applying the patch below (the patch is based on the CuPy implementation of `cupyx.signal`, see https://github.com/cupy/cupy/blob/v11.5.0/cupyx/scipy/signal/_signaltools.py...), and running `$ python dev.py test -s signal`, I only get three failures in `medfilt`, related to - object arrays of `Decimal` instances - `longdouble` arrays.
So, assuming we can deprecate longdoubles and decimals in these filters, we could potentially remove some 700 lines of obscure C code in https://github.com/scipy/scipy/blob/main/scipy/signal/_lfilter.c.in
Thoughts?
+1 this sounds like a healthy idea. There's a lot of duplication in these types of routines, and it's quite under-maintained code. And these deprecations sound perfectly fine.
Cheers, Ralf
Evgeni
P.S. The patch to check the mapping between signal.{order,median}_filter and ndimage.rank_filter:
```diff diff --git a/scipy/signal/_signaltools.py b/scipy/signal/_signaltools.py index 31baba2421..2f00ead3a2 100644 --- a/scipy/signal/_signaltools.py +++ b/scipy/signal/_signaltools.py @@ -1492,7 +1492,15 @@ def order_filter(a, domain, rank): if (dimsize % 2) != 1: raise ValueError("Each dimension of domain argument " "should have an odd number of elements.") - return _sigtools._order_filterND(a, domain, rank) + + result = _sigtools._order_filterND(a, domain, rank) + + import scipy.ndimage as ndimage + import numpy.testing as testing + res_ndi = ndimage.rank_filter(a, rank, footprint=domain, mode='constant') + testing.assert_equal(result, res_ndi) + + return result
def medfilt(volume, kernel_size=None): @@ -1555,7 +1563,17 @@ def medfilt(volume, kernel_size=None):
numels = np.prod(kernel_size, axis=0) order = numels // 2 - return _sigtools._order_filterND(volume, domain, order) + result = _sigtools._order_filterND(volume, domain, order) + + + import scipy.ndimage as ndimage + import numpy.testing as testing + size = np.prod(kernel_size) + res_ndi = ndimage.rank_filter(volume, size // 2, size=kernel_size, + mode='constant') + testing.assert_equal(result, res_ndi) + + return result
def wiener(im, mysize=None, noise=None): ``` _______________________________________________ SciPy-Dev mailing list -- scipy-dev@python.org To unsubscribe send an email to scipy-dev-leave@python.org https://mail.python.org/mailman3/lists/scipy-dev.python.org/ Member address: ralf.gommers@googlemail.com
_______________________________________________ SciPy-Dev mailing list -- scipy-dev@python.org To unsubscribe send an email to scipy-dev-leave@python.org https://mail.python.org/mailman3/lists/scipy-dev.python.org/ Member address: evgeny.burovskiy@gmail.com
participants (2)
-
Evgeni Burovski -
Ralf Gommers