Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
asmeurer opened this issue Dec 6, 2023 · 6 comments · Fixed by #720 · May be fixed by #885
Open

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

asmeurer opened this issue Dec 6, 2023 · 6 comments · Fixed by #720 · May be fixed by #885
Labels
Accepted RFC feature request which has been accepted. RFC Request for comments. Feature requests and proposed changes. topic: FFT Fast Fourier transforms.
Milestone

Comments

@asmeurer
Copy link
Member

asmeurer commented Dec 6, 2023

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
Copy link
Member Author

asmeurer commented Dec 6, 2023

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
Copy link
Member Author

asmeurer commented Dec 6, 2023

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
Copy link
Member

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
Copy link
Contributor

leofang commented Dec 12, 2023

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.

@rgommers rgommers added the topic: FFT Fast Fourier transforms. label Jan 11, 2024
@asmeurer
Copy link
Member Author

asmeurer commented Feb 5, 2024

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
Copy link
Contributor

leofang commented Feb 7, 2024

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

kgryte added a commit that referenced this issue Feb 8, 2024
PR-URL: 	#720
Closes: #717
Closes: #718
Ref: #189
Co-authored-by: Leo Fang <[email protected]>
Co-authored-by: Athan Reines <[email protected]>
Reviewed-by: Leo Fang <[email protected]>
Reviewed-by: Athan Reines <[email protected]>
Reviewed-by: Ralf Gommers <[email protected]>
@asmeurer asmeurer reopened this Feb 8, 2024
@kgryte kgryte reopened this Feb 8, 2024
@leofang leofang added this to the v2023 milestone Feb 8, 2024
@kgryte kgryte modified the milestones: v2023, v2024 Feb 19, 2024
@kgryte kgryte changed the title Return dtype for fftfreq and rfftfreq RFC: add support for a dtype kwarg to fftfreq and rfftfreq Apr 4, 2024
@kgryte kgryte added RFC Request for comments. Feature requests and proposed changes. Accepted RFC feature request which has been accepted. labels Apr 4, 2024
kgryte added a commit to kgryte/array-api that referenced this issue Jan 20, 2025
@kgryte kgryte linked a pull request Jan 20, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted RFC feature request which has been accepted. RFC Request for comments. Feature requests and proposed changes. topic: FFT Fast Fourier transforms.
Projects
None yet
4 participants