ENH: signal.windows: add array API support (take 2) #21783
Conversation
573eb55 to
65bdc5d
Compare
b27d82b to
7a6e164
Compare
scipy/signal/windows/_windows.py
Outdated
| dpss_rxx = _fftautocorr(xp.asarray(windows)) | ||
| r = 4 * W * xp_sinc(xp.asarray(2 * W * nidx), xp=xp) | ||
| r[0] = 2 * W | ||
| ratios = np.dot(dpss_rxx, r) | ||
| ratios = xp.matmul(dpss_rxx, r) | ||
| if singleton: | ||
| ratios = ratios[0] | ||
| ratios = xp.asarray(ratios, device=device) |
There was a problem hiding this comment.
can we do all of this calculation on device rather than copying at the end?
There was a problem hiding this comment.
There's not much point ATM because dpss is effectively CPU-only; it relies on eigh_triangular which is not available on CUDA anyhow.
|
Updated to address review comments. |
68d2c37 to
775ab2d
Compare
|
CI failures are data-apis/array-api-extra#49 |
|
The general discussion on the default fp dtype seems to have My summary of actionable items is #22210 To help move this PR forward, I rolled back the dtype changes and fixed the CI. Thus, the CI here is green with no sweeping changes to unrelated modules. |
|
CI failures here are an interesting fallout from #22132 Here's a test which looks like this: Thoughts @crusaderky ? |
|
If you're comparing to a python scalar then I think you can just use |
xp_assert_close(PSLL, -35.1672, atol=1, xp=np) |
|
The CI is all green, all review comments are addressed either here or separately; I'm going to press the green button in about a week's time unless there are further comments. |
| ) | ||
|
|
||
| def test_correctness(self): | ||
| @skip_xp_backends(cpu_only=True) |
There was a problem hiding this comment.
Nit, it would be nice to have a reason
There was a problem hiding this comment.
No fundamental reason, just that the rest of the test is using f_np = np.asarray(f) . Want me to flush the CI for it?
There was a problem hiding this comment.
Probably not, maybe just add it in one of the subsequent prs if you remember
| assert_allclose(PSLL, -35.1672, atol=1) | ||
| assert_allclose(BW_3dB, 1.1822, atol=0.1) | ||
| assert_allclose(BW_18dB, 2.6112, atol=0.1) | ||
| assert math.isclose(PSLL, -35.1672, abs_tol=1) |
There was a problem hiding this comment.
Did we need to change anything here since it looks like all the qualities being test come from f_np? It's just I imagine assert math.isclose(...) has a less informative error
There was a problem hiding this comment.
Ah, this :-). Yes, since #22132 you cannot xp_assert_close(np_array, np_array) when xp is not numpy.
An alternative is, I believe, xp_assert_close(actual, desired, xp=array_namespace(np.empty(0))) or some such.
There was a problem hiding this comment.
Can we not just leave it as assert_allclose?
There was a problem hiding this comment.
FWIW I prefer to introduce math.isclose to reintroducing numpy.testing.assert_allclose, for python scalars.
Reference issue
supersedes #20668
What does this implement/fix?
Finish up gh-20688 : make
signal.windowsarray api compatible.Had to add several small shims:
acos / arccosrenames in numpy 2.0; We'll be able to drop the shims when we drop numpy < 2 and cupy < 14asarrayaccepting / not accepting thedevice=kwargxp_assert_*functions: ifdesiredis a list, deduce its array namespace fromarray_namespace(actual).The rest is fairly straightforward
cc @jakevdp : can push additional commits into your branch if you prefer.
Additional information
Needed to add delegation to several signal functions which only accepts scalars, not arrays; wanted to follow the prior art, found gh-20668; noticed that the CI is red and logs are gone; and here we are, several frustrating hours later.