ENH: signal.window: add array API support #20668
Conversation
1490abb to
e2d1e1c
Compare
There was a problem hiding this comment.
the diff here looks fine to me, thanks! Would you like to start converting some of the tests? See gh-19001, or any of the recently merged stats PRs labelled with array types for examples.
We should let @rgommers weigh in on whether to name the parameter array_namespace or xp, and whether we would like a device parameter as well.
|
Thanks for this @jakevdp!
We should do this consistently for all functions that create new arrays without receiving an array input. For fftfreq & co, we went with two keyword-only keywords: |
|
Thanks @rgommers – I'll update this to use add the I can also work on refactoring the tests, but perhaps we should do that in a separate PR: it always worries me to do significant refactoring of tests and code at the same time, because it may mask changes in user-visible behavior! |
|
I hadn't considered that argument for not considering the tests yet - you make a good point. So far I don't think we've accidentally introduced any regressions because of test changes. A separate PR seems like a potentially useful strategy, as long as it's available together with the code changes. Otherwise we have no way of testing the new functionality. How about opening a second PR on top of this one including test changes? Then we can choose to either:
|
That sounds like the right idea to me. |
|
I updated |
|
I tried converting the test but it's failing, I think because the |
|
Right, this is why I didn't do this in |
|
Should we worry about no longer having test coverage for the default call pattern, i.e. not passing |
I would add a separate test for just that. |
| @@ -53,6 +54,10 @@ def general_cosine(M, a, sym=True): | |||
| When True (default), generates a symmetric window, for use in filter | |||
| design. | |||
| When False, generates a periodic window, for use in spectral analysis. | |||
| xp : module, optionaloptional array API namespace (default: numpy) | |||
There was a problem hiding this comment.
| xp : module, optionaloptional array API namespace (default: numpy) | |
| xp : module, optional | |
| array API namespace (default: numpy) |
Just a minor typo
|
(marked as draft until we have the PR with the test changes) |
Towards gh-20678.
This PR adds Array API standard support to window function definitions within
scipy.signal.This is a bit different than other Array API changes, becuase these functions construct arrays from scratch. So rather than pulling the array API namespace from the inputs, this adds a new optional
array_namespacekeyword argument to each function that lets the user specify an array API; this defaults toscipy._lib.array_api_compat.numpy, so there shouldn't be any user-visible change in the default case.This should be considered a draft – I'm curious if maintainers have thoughts about whether this keyword argument is the best way to support the Array API for cases like this. Thanks!