EHN: signal.window: add array API support

[jakevdp]

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_namespace keyword argument to each function that lets the user specify an array API; this defaults to scipy._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!

[lucascolley]

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.

[rgommers]

Thanks for this @jakevdp!

whether to name the parameter array_namespace or xp, and whether we would like a device parameter as well.

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: xp=None, device=None. Which still seems fine to me, so I suggest to go with that. xp is indeed very slightly non-descriptive, but I think if we use it consistently it will become quite recognizable to users soon enough.

[jakevdp]

Thanks @rgommers – I'll update this to use add the xp and device arguments today.

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!

[rgommers]

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:

• merge this PR, then rebase and merge the second PR, or
• simply consider this one as thorough regression testing, and only merge the second PR when both are green (closing this PR)

[lucascolley]

How about opening a second PR on top of this one including test changes? Then we can choose to either:

merge this PR, then rebase and merge the second PR, or
simply consider this one as thorough regression testing, and only merge the second PR when both are green (closing this PR)

That sounds like the right idea to me.

[jakevdp]

I updated _windows.py with the xp and device arguments, and squashed the commit. I'll plan to work on test changes in a second commit on top of that, probably some time next week.

[jakevdp]

I tried converting the test but it's failing, I think because the xp fixture tries to pass in numpy, which is not (currently) compatible with the Array API (e.g. np.concat doesn't exist, np.array doesn't support the device argument, etc.). What's the best approach here?

[lucascolley]

Right, this is why I didn't do this in fftfreq. It shouldn't be too complicated though, I think xp_res = array_namespace(xp) in the test functions, then passing xp=xp_res in to the functions being tested, should work.

[jakevdp]

Should we worry about no longer having test coverage for the default call pattern, i.e. not passing xp at all?

[lucascolley]

Should we worry about no longer having test coverage for the default call pattern, i.e. not passing xp at all?

I would add a separate test for just that.

[Kai-Striega]

Suggested change

	xp : module, optionaloptional array API namespace (default: numpy)
	xp : module, optional
	array API namespace (default: numpy)

Just a minor typo