BUG: Fix sinc for custom objects #29580
Conversation
cdedc3b to
728bd04
Compare
Numpy 2.3 broke the sinc function for custom objects because of an explicit dtype check for determining the appropriate eps. This PR fixes this issue by more introducing a more cautious check.
728bd04 to
d9913da
Compare
| if hasattr(x, "dtype") and getattr(x.dtype, "kind", None) == "f": | ||
| eps = np.finfo(x.dtype).eps | ||
| else: | ||
| eps = 1e-20 |
There was a problem hiding this comment.
I suppose if it is worth fixing it is worth adding a regression test inspired by your reproducer like below the fold.
--- a/numpy/lib/tests/test_function_base.py
+++ b/numpy/lib/tests/test_function_base.py
@@ -2363,6 +2363,19 @@ def test_masked(self):
class TestSinc:
+ def test_sinc_obj(self):
+ class MyObject:
+
+ def __init__(self, value):
+ self.value = value
+
+ def __rmul__(self, y):
+ return self.value * y
+
+ number = MyObject(2)
+ assert_allclose(np.sinc(number), np.sinc(2))But maybe best to wait for feedback from a more active dev before putting more time into it.
There was a problem hiding this comment.
Yeah, we should definitely add a test. I do still wonder if we can find a slightly nicer work-around for backporting, but can't think of one.
|
This way of using If we were to actively restore support for this hack, and add regression tests for it, then we're effectively adding official support for it. That means that would also have to document it, and modify the typing stubs to reflect this. But because I haven't seen a convincing use-case, and am not able to think of one either, I don't think that we should. And even if we wouldn't document it, and just keep it something like an "informal feature", then we risk setting a precedent for hacks like these. So long story short; I don't think we should let |
|
Thanks a lot for your feedback, I agree that this is a bit of hack and not really worth being officially supported. I still think that the current way of determining np.sinc(np.array(1.0, dtype=object))fails with the unintuitive Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File ".../python3.12/site-packages/numpy/lib/_function_base_impl.py", line 3852, in sinc
eps = np.finfo(x.dtype).eps if x.dtype.kind == "f" else 1e-20
^^^^^^^
AttributeError: 'float' object has no attribute 'dtype'while I would expect this behaviour >>> np.sinc(np.array([1.0], dtype=object))
AttributeError: 'float' object has no attribute 'sin'
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File ".../python3.12/site-packages/numpy/lib/_function_base_impl.py", line 3854, in sinc
return sin(y) / y
^^^^^^
TypeError: loop of ufunc does not support argument 0 of type float which has no callable sin methodNot a big issue, though, so feel free to close the PR 👍 |
|
We should fix this, but by using EIDT: Although, dunno, there is some risk in backporting that, but this might be a bit annoying regression also. |
This PR introduces a simple fix for the sinc function which stopped working for minimal custom objects. The problem was introduced in #27784 / numpy 2.3 and is caused by an explicit check for the kind of the associated dtype in order to determine an appropriate eps. This PR makes the check more robust.
Minimal breaking example:
Fails with