search

data-apis / array-api

RFC document, tooling and other content related to the array API standard
https://data-apis.github.io/array-api/latest/
MIT License
205 stars 42 forks source link

RFC: add support for a `dtype` kwarg to `fftfreq` and `rfftfreq` #717

Open asmeurer opened 7 months ago

asmeurer commented 7 months ago

The specs for fftfreq and rfftfreq say

"The returned array must have a real-valued floating-point data type determined by Type Promotion Rules."

But there is no type promotion here, since none of the input arguments are arrays. Presumably it should say that the return type should have the default real floating-point dtype.

But that also begs the question of whether these functions should have a dtype argument like the other array creation functions.

asmeurer commented 7 months ago

Actually all the fft functions have this "type promotion" line, except for fftshift and ifftshift, which say "The returned array must have the same data type as x." But each of these functions only have one array argument, so the "same data type" line is more appropriate.

asmeurer commented 7 months ago

Actually all the fft functions have this "type promotion" line, except for fftshift and ifftshift, which say "The returned array must have the same data type as x." But each of these functions only have one array argument, so the "same data type" line is more appropriate.

Oh I see it's because it's promoting from real to complex. It's a bit odd to call this type promotion, though, and it's even more awkward for functions like irfft that go from complex to real. It would be better to say something like "a complex floating-point dtype with the same bit width as x" (I don't know if we already have some wording like that).

rgommers commented 6 months ago

That sounds like an improvement - although I'd clarify the "same bit width" phrase a bit, since it'd be float64 -> complex128 (so the dtype has double the number of bits).

leofang commented 6 months ago

Sorry, Aaron, for some reason I missed this issue. My bad.

For all applications that I know of (mainly in signal processing where fftfreq is used to prepare signal filters/windows), they must stay real. I think if (r)fftfreq were to return complex arrays, they'd break SciPy and Matplotlib in some unexpected ways.

IIRC @oleksandr-pavlyk raised the same question about how return real types should be determined, and the argument back then involved viewing these two functions as constructors. I think one could argue the dtypes can be determined by the argument d that specifies the step size, but adding an extra dtype argument to offer explicit control seems reasonable to me.

asmeurer commented 5 months ago

My point for adding a dtype argument wasn't to allow the user to specify a complex dtype. That should probably disallowed (or unspecified?). It was to allow the user to specify 32-bit vs. 64-bit, for consistency with the remaining creation functions in the spec.

I don't think we can get the dtype from d, which is only specified as a float. The only option is to return the default real floating dtype, which is what is specified at https://github.com/data-apis/array-api/pull/720/files#diff-31eaf0d090ecc4df2af89ab3d3a33b91ff8846925b579fd64bf065aea146a230R612 (prior to that PR it was just an ambiguous "a real-valued floating-point data type").

Given that this would be a new API, and no one has requested this, I would suggest making this change for 2024. We are already close to tagging 2023. And furthermore, this would not be compatible with the current NumPy API, which does not have a dtype argument in fftfreq or rfftfreq. If we were to squeeze this in for 2023, we would also want to get that added to NumPy for numpy 2.0 (which is probably possible, but again, no one has actually requested this yet, so it's not a priority).

leofang commented 4 months ago

Sounds good to me, I have no concern for either v2023 or v2024.