Fix dpnp.tensor.acosh handling of complex(+-0, NaN)#2914
Fix dpnp.tensor.acosh handling of complex(+-0, NaN)#2914vlad-perevezentsev wants to merge 6 commits into
dpnp.tensor.acosh handling of complex(+-0, NaN)#2914Conversation
|
View rendered docs @ https://intelpython.github.io/dpnp/pull/2914/index.html |
|
Array API standard conformance tests for dpnp=0.21.0dev0=py313h509198e_23 ran successfully. |
|
|
||
| Y_dpt = dpt.asnumpy(dpt.acosh(yf)) | ||
|
|
||
| for i in range(len(x)): |
There was a problem hiding this comment.
we need to place a comment that NumPy returns wrong result: NaN + NaN j
and probably to add a link to the issue in NumPy repo
There was a problem hiding this comment.
conda-forge numpy gives the nan, nan result, while wheels give the 0, nan result, because of something to do with numpy dispatching
probably, it would be best to just not use numpy for testing these kinds of edge case results
There was a problem hiding this comment.
Yes, I have noticed that too
It is better not to compare it to NumPy at all
Thank you!
There was a problem hiding this comment.
But both cases seem invalid, we expect NaN ± π/2 j based on the spec. Should we report then?
There was a problem hiding this comment.
But both cases seem invalid, we expect
NaN ± π/2 jbased on the spec. Should we report then?
PyPI isn't incorrect
In [1]: import numpy as np
In [2]: vals = [complex(np.nan, -np.inf), complex(-np.inf, np.nan), complex(0, np.nan)]
In [3]: x = np.array(vals)
In [4]: np.arcsinh(x[0]), np.arcsin(x[1]), np.arccosh(x[2])
Out[4]:
(np.complex128(inf+nanj),
np.complex128(nan+infj),
np.complex128(nan+1.5707963267948966j))
the last case is NaN + pi/2, it's only conda-forge which is incorrect.
I don't think we shouldn't file, just noting that, they are transiently aware of it and it's platform-dependent for NumPy failure
There was a problem hiding this comment.
I see, I thought the same faulty result for PyPI.
| pi_half = np.full(len(x), np.pi / 2, dtype=xf.real.dtype) | ||
| assert np.all(np.isnan(Y_dpt.real)) | ||
| assert_allclose(abs(Y_dpt.imag), pi_half, atol=1e-6) |
There was a problem hiding this comment.
I guess it might be simplified:
| pi_half = np.full(len(x), np.pi / 2, dtype=xf.real.dtype) | |
| assert np.all(np.isnan(Y_dpt.real)) | |
| assert_allclose(abs(Y_dpt.imag), pi_half, atol=1e-6) | |
| assert np.isnan(Y_dpt.real).all() | |
| assert_allclose(np.abs(Y_dpt.imag), np.pi / 2, atol=1e-6) |
antonwolfy
left a comment
There was a problem hiding this comment.
The only minor nit in the tests. The remaining is absolutely LGTM!
Thank you @vlad-perevezentsev
This PR proposes to fix issue #2880 where
dpnp.tensor.acosh(complex(0, NaN))returnedNaN + NaN jinstead ofNaN ± π/2 jas required by the Array API specificationThe issue was caused by an incorrect early-return branch in acosh.hpp:
This special case bypassed the general logic which already produces the correct result.
The fix removes this unnecessary branch making the behavior consistent with both the Array API specification and NumPy
Also adds a new
test_acosh_zero_nantest to cover this case