DEP: signal: raise error using medfilt and order_filter with float128 and object dtypes #19673

j-bowhay · 2023-12-10T22:06:49Z

Reference issue

follow up to #18341

What does this implement/fix?

It has been two releases since float128 and object types were deprecated to deduplicate code, should be fine to raise an error now.

Additional information

… and object dtypes

j-bowhay · 2023-12-10T22:08:32Z

scipy/signal/tests/test_signaltools.py

@@ -1115,7 +1106,7 @@ def test_invalid_dtypes(self, dtype):
    def test_none(self):
        # gh-1651, trac #1124. Ensure this does not segfault.
        with pytest.warns(UserWarning):
-            assert_raises(TypeError, signal.medfilt, None)
+            assert_raises(ValueError, signal.medfilt, None)


Looks like there was some sort of special casing here for None. Have it lumped in with the others that raise ValueError but could special case so this remains a TypeError

scipy/signal/tests/test_signaltools.py

h-vetinari

There's a mismatch between the PR title and the scope of what it does.

scipy/signal/_signaltools.py

scipy/signal/_sigtoolsmodule.c

lucascolley · 2023-12-18T23:36:58Z

I'm also a bit confused since gh-18341 said "The goal here is to reduce duplication between ndimage and signal, and get rid of https://github.com/scipy/scipy/blob/main/scipy/signal/_lfilter.c.in entirely (700 LOC of fairly obscure C code). Cannot just remove them, so deprecate first."

j-bowhay · 2023-12-19T09:39:32Z

The point of the original deprecation was that we currently have two sets of C code for doing the same thing, one in signal and one in ndimage. The signal functions were modified to use the ndimage code under the hood so signal C code could be removed as it was decided the ndimage code was more maintainable. However ndimage did not support object or float128 dtypes hence the deprecation cycle before all the signal C code could be removed.

j-bowhay · 2023-12-19T09:41:11Z

There are some relevant test failures that need resolving though

lucascolley · 2023-12-19T10:45:15Z

Ah okay, so does all of _lfilter.c.in need to stay?

j-bowhay · 2023-12-19T10:55:44Z

Ah okay, so does all of _lfilter.c.in need to stay?

As far as I could tell it was still needed but I also might have just got lost the maze. Maybe @ev-br could confirm?

rgommers · 2023-12-19T12:35:59Z

scipy/signal/_signaltools.py

    if volume.dtype in [np.bool_, np.complex64, np.complex128, np.clongdouble,
-                        np.float16]:
+                        np.float16, np.object_, 'float128']:


Note that you want to use longdouble as the dtype name, because that's the canonical one (float128 may not exist, and then the alias float96 will exist)

ev-br · 2023-12-19T13:09:05Z

Ah okay, so does all of _lfilter.c.in need to stay?

Indeed. Thank you Jake for chasing down where this dead code is despite the typo in the original PR which introduced the deprecation.

j-bowhay · 2023-12-19T14:24:42Z

Updated to use longdouble and removed more dead code

h-vetinari

Some relatively minor nits, otherwise looking good (thanks for the context on _order_filterND).

scipy/signal/_signaltools.py

scipy/signal/tests/test_signaltools.py

h-vetinari

Last details. Not sure why the doc build is failing, but at least win/macos jobs need to be fixed.

h-vetinari · 2024-01-13T02:50:09Z

scipy/signal/_signaltools.py

+    if volume.dtype in [np.bool_, np.complex64, np.complex128, np.clongdouble,
+                        np.float16, np.object_, np.longdouble]:
+        raise ValueError(f"dtype={volume.dtype} is not supported by medfilt")


I think np.longdouble here is incorrect, because on windows that just ends up being float64 and then causes

FAILED scipy\signal\tests\test_signaltools.py::TestMedFilt::test_types[float64] - ValueError: dtype=float64 is not supported by medfilt

IMO it would be better to formulate this as:

Suggested change

if volume.dtype in [np.bool_, np.complex64, np.complex128, np.clongdouble,

np.float16, np.object_, np.longdouble]:

raise ValueError(f"dtype={volume.dtype} is not supported by medfilt")

if volume.dtype not in <list_of_allowed_types>:

raise ValueError(f"dtype={volume.dtype} is not supported by medfilt")

h-vetinari

Almost there, but still not yet. 🙃

h-vetinari · 2024-01-13T08:21:38Z

scipy/signal/_signaltools.py

+    if volume.dtype in [np.bool_, np.complex64, np.complex128, np.clongdouble,
+                        np.float16, np.object_, "float128", "float96"]:
+        raise ValueError(f"dtype={volume.dtype} is not supported by medfilt")


I would still prefer explicitly naming the types for which we have support in medfilt (non-binding).

If starting from scratch I would entirely agree with you however I am slightly hesitant to do this as, given all the longdouble confusion, I think it might be quite easy to mess up and introduce a change that was not within the scope of the original deprecation.

My point is that it should be trivial to specify the types we support - those are the one we actually implement (resp. template over), and that's IMO easier and more useful than playing whack-a-mole with weird/rare types that we somehow forgot to mention here.

Thanks for picking this up again! I just looked through again and it looks good, though I'd still want to allowlist certain types here, rather than blocklist an arbitrarily long list of dtypes (as an example taken at random, we're probably not failing correctly for datetime types)

Thanks for taking a look, hopefully this has been addressed in the latest commit

h-vetinari · 2024-01-13T08:27:15Z

scipy/signal/tests/test_signaltools.py

+        if in_typed.dtype is np.float64:
+            pytest.mark.skip("Platform does not support `float128`")


The combination of if-condition and skip-message ~~makes no sense~~ is confusing. It would be much better not to parametrize on an ambiguous type (np.longdouble) in the first place, but if that's unavoidable, one way to clarify would be (feel free to rewrite):

Suggested change

if in_typed.dtype is np.float64:

pytest.mark.skip("Platform does not support `float128`")

if in_typed.dtype is np.float64:

pytest.mark.skip("Platform treats `np.longdouble` as `np.float64`; "

"we support the latter, but not the former (in general).")

Agh, the tests are still failing on windows and macos arm, neither of which I have access to, so this clearly is not quite right. @ev-br would you be able to point me in the right direction here as I am getting quite confused!

I don't think if dtype is np.float64 is correct really. I suspect you're hitting #18341 (comment), so can you dispatch on in_typed.dtype.char == 'g' instead?

I can check it in a couple of days unless you beat me to it.

Or just don't parametrize the test with np.longdouble, but rather float96 & float128.

Maybe I am misunderstanding but I don't think we can use float128 directly here as it doesn't exist on some platforms.

Yes, you'll need to skip it on platforms that don't have it. I made another suggestion that's IMO easier to follow (and allowing to delete the if...skip after in_typed entirely), because there's no need to invert any logic (of the kind "we support everything but these types" or "we support np.longdouble except when it's actually np.float64).

h-vetinari · 2024-01-13T22:51:09Z

scipy/signal/tests/test_signaltools.py

+                                       np.clongdouble, np.float16, np.longdouble,
+                                       np.object_])
    def test_invalid_dtypes(self, dtype):
        in_typed = np.array(self.IN, dtype=dtype)


Suggested change

np.clongdouble, np.float16, np.longdouble,

np.object_])

def test_invalid_dtypes(self, dtype):

in_typed = np.array(self.IN, dtype=dtype)

np.clongdouble, np.float16,

np.object_, "float96", "float128"])

def test_invalid_dtypes(self, dtype):

if system_does_not_have_large_float and dtype in ["float96", "float128"]:

pytest.mark.skip("Platform does not support floating point types larger than 64bit")

in_typed = np.array(self.IN, dtype=dtype)

system_does_not_have_large_float is left as an exercise to the reader. ;-)

h-vetinari · 2024-01-13T22:53:53Z

scipy/signal/tests/test_signaltools.py

+        if in_typed.dtype is np.float64:
+            pytest.mark.skip("Platform does not support `float128`")


Yes, you'll need to skip it on platforms that don't have it. I made another suggestion that's IMO easier to follow (and allowing to delete the if...skip after in_typed entirely), because there's no need to invert any logic (of the kind "we support everything but these types" or "we support np.longdouble except when it's actually np.float64).

h-vetinari · 2024-01-17T03:58:00Z

I think we can wait for 1.14 with this? Unless you want to get this in urgently, I think we should give it a year as usual.

j-bowhay · 2024-01-17T08:20:07Z

Closing until we branch for 1.14

h-vetinari · 2024-03-19T04:21:30Z

Branch has happened, reopening.

h-vetinari · 2024-04-17T00:55:46Z

@j-bowhay, do you think you'll be able to rebase this in the coming weeks?

scipy/signal/_signaltools.py

scipy/signal/tests/test_signaltools.py

[skip ci] Co-authored-by: h-vetinari <h.vetinari@gmx.com>

h-vetinari

Thank you!

h-vetinari · 2024-05-23T04:48:21Z

@j-bowhay, do you want to keep some of the commit granularity (would probably need a clean-up), or are you fine with a squash-merge?

j-bowhay · 2024-05-23T07:24:54Z

@h-vetinari happy to squash merge

DEP: signal: raise error using medfilt and order_filter with float128…

b6ed40a

… and object dtypes

j-bowhay added scipy.signal deprecated Items related to behavior that has been deprecated labels Dec 10, 2023

j-bowhay requested a review from ev-br December 10, 2023 22:06

j-bowhay requested review from larsoner and ilayn as code owners December 10, 2023 22:06

j-bowhay commented Dec 10, 2023

View reviewed changes

scipy/signal/tests/test_signaltools.py Show resolved Hide resolved

j-bowhay mentioned this pull request Dec 10, 2023

META: deprecation tracker #15765

Open

29 tasks

j-bowhay added this to the 1.13.0 milestone Dec 10, 2023

MAINT: fix float128 issue

7749a90

h-vetinari reviewed Dec 18, 2023

View reviewed changes

scipy/signal/_signaltools.py Show resolved Hide resolved

scipy/signal/_sigtoolsmodule.c Outdated Show resolved Hide resolved

rgommers reviewed Dec 19, 2023

View reviewed changes

ev-br mentioned this pull request Dec 19, 2023

DEP: signal: deprecate using medfilt and order_filter with float128 and object dtypes #18341

Merged

j-bowhay added 2 commits December 19, 2023 14:16

MAINT: use longdouble instead of float128

833e5e8

MAINT: remove more unused C code

9a6a159

h-vetinari reviewed Dec 26, 2023

View reviewed changes

scipy/signal/_signaltools.py Outdated Show resolved Hide resolved

scipy/signal/tests/test_signaltools.py Outdated Show resolved Hide resolved

scipy/signal/tests/test_signaltools.py Outdated Show resolved Hide resolved

j-bowhay added 3 commits January 12, 2024 21:38

Merge branch 'main' into scipygh-18341-follow-up

6aa3bae

STY: fix indentation

882ab20

MAINT/TST: modernise test and more error earlier

e8f917c

h-vetinari reviewed Jan 13, 2024

View reviewed changes

MAINT: switch from longdouble to float128/float96

5713ab4

j-bowhay force-pushed the gh-18341-follow-up branch from 948529e to 5713ab4 Compare January 13, 2024 07:35

h-vetinari reviewed Jan 13, 2024

View reviewed changes

h-vetinari mentioned this pull request Jan 17, 2024

DEP: extend some announced deprecations due to out-of-band 1.13 release #19892

Merged

j-bowhay closed this Jan 17, 2024

j-bowhay removed this from the 1.13.0 milestone Jan 17, 2024

h-vetinari reopened this Mar 19, 2024

j-bowhay mentioned this pull request Apr 17, 2024

MAINT: remove incidental imports from private modules #19904

Merged

j-bowhay added the needs-work Items that are pending response from the author label Apr 26, 2024

j-bowhay added this to the 1.14.0 milestone Apr 26, 2024

j-bowhay added 3 commits May 18, 2024 13:18

Merge branch 'main' into scipygh-18341-follow-up

2dabe87

update test to skip depending on what longdouble the system has

57be4b2

specify dtypes that are allowed

50c5509

j-bowhay removed the needs-work Items that are pending response from the author label May 18, 2024

j-bowhay requested a review from h-vetinari May 21, 2024 09:10

h-vetinari reviewed May 21, 2024

View reviewed changes

scipy/signal/_signaltools.py Outdated Show resolved Hide resolved

h-vetinari reviewed May 21, 2024

View reviewed changes

scipy/signal/tests/test_signaltools.py Outdated Show resolved Hide resolved

j-bowhay added 2 commits May 21, 2024 11:44

improve order_filter dtype check

4eb4846

add comment explaining test skip

5023b01

h-vetinari reviewed May 21, 2024

View reviewed changes

scipy/signal/tests/test_signaltools.py Outdated Show resolved Hide resolved

Update scipy/signal/tests/test_signaltools.py

2a7971e

[skip ci] Co-authored-by: h-vetinari <h.vetinari@gmx.com>

h-vetinari approved these changes May 21, 2024

View reviewed changes

h-vetinari merged commit b421cd6 into scipy:main May 23, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEP: signal: raise error using medfilt and order_filter with float128 and object dtypes #19673

DEP: signal: raise error using medfilt and order_filter with float128 and object dtypes #19673

j-bowhay commented Dec 10, 2023

j-bowhay Dec 10, 2023

h-vetinari left a comment

lucascolley commented Dec 18, 2023

j-bowhay commented Dec 19, 2023

j-bowhay commented Dec 19, 2023

lucascolley commented Dec 19, 2023

j-bowhay commented Dec 19, 2023

rgommers Dec 19, 2023

ev-br commented Dec 19, 2023

j-bowhay commented Dec 19, 2023

h-vetinari left a comment

h-vetinari left a comment

h-vetinari Jan 13, 2024

h-vetinari left a comment

h-vetinari Jan 13, 2024

j-bowhay Jan 13, 2024

h-vetinari Jan 13, 2024

h-vetinari May 18, 2024

j-bowhay May 18, 2024

h-vetinari Jan 13, 2024 •

edited

j-bowhay Jan 13, 2024 •

edited

ev-br Jan 13, 2024 •

edited

h-vetinari Jan 13, 2024

j-bowhay Jan 13, 2024

h-vetinari Jan 13, 2024 •

edited

h-vetinari Jan 13, 2024 •

edited

h-vetinari Jan 13, 2024 •

edited

h-vetinari commented Jan 17, 2024

j-bowhay commented Jan 17, 2024

h-vetinari commented Mar 19, 2024

h-vetinari commented Apr 17, 2024

h-vetinari left a comment

h-vetinari commented May 23, 2024

j-bowhay commented May 23, 2024

		if in_typed.dtype is np.float64:
		pytest.mark.skip("Platform does not support `float128`")

DEP: signal: raise error using medfilt and order_filter with float128 and object dtypes #19673

DEP: signal: raise error using medfilt and order_filter with float128 and object dtypes #19673

Conversation

j-bowhay commented Dec 10, 2023

Reference issue

What does this implement/fix?

Additional information

Choose a reason for hiding this comment

h-vetinari left a comment

Choose a reason for hiding this comment

lucascolley commented Dec 18, 2023

j-bowhay commented Dec 19, 2023

j-bowhay commented Dec 19, 2023

lucascolley commented Dec 19, 2023

j-bowhay commented Dec 19, 2023

Choose a reason for hiding this comment

ev-br commented Dec 19, 2023

j-bowhay commented Dec 19, 2023

h-vetinari left a comment

Choose a reason for hiding this comment

h-vetinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-vetinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-vetinari Jan 13, 2024 • edited

Choose a reason for hiding this comment

j-bowhay Jan 13, 2024 • edited

Choose a reason for hiding this comment

ev-br Jan 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-vetinari Jan 13, 2024 • edited

Choose a reason for hiding this comment

h-vetinari Jan 13, 2024 • edited

Choose a reason for hiding this comment

h-vetinari Jan 13, 2024 • edited

Choose a reason for hiding this comment

h-vetinari commented Jan 17, 2024

j-bowhay commented Jan 17, 2024

h-vetinari commented Mar 19, 2024

h-vetinari commented Apr 17, 2024

h-vetinari left a comment

Choose a reason for hiding this comment

h-vetinari commented May 23, 2024

j-bowhay commented May 23, 2024

h-vetinari Jan 13, 2024 •

edited

j-bowhay Jan 13, 2024 •

edited

ev-br Jan 13, 2024 •

edited

h-vetinari Jan 13, 2024 •

edited

h-vetinari Jan 13, 2024 •

edited

h-vetinari Jan 13, 2024 •

edited