Skip to content

Add FFT extension to the spec #189

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 37 commits into from
Oct 20, 2022
Merged

Add FFT extension to the spec #189

merged 37 commits into from
Oct 20, 2022

Conversation

steff456
Copy link
Member

@steff456 steff456 commented Jun 4, 2021 •
edited by leofang
Loading

This PR focuses on adding the FFT as an extension to the array spec.

The current APIs added in this PR are:

  • Standard FFT: fft, ifft, fftn, ifftn
  • Real FFT: rfft, irfft, rfftn, irfftn
  • Hermitian FFT: hfft, ihfft
  • Helper Routines: fftfreq, rfftfreq, fftshift, ifftshift
  • Update to rst and create function stubs

They follow the summary available at #159. Close #476.

@steff456 steff456 self-assigned this Jun 4, 2021
@rgommers rgommers added the API extension Adds new functions or objects to the API. label Jun 7, 2021
rgommers
rgommers reviewed Jun 7, 2021
View reviewed changes
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @steff456, this looks good overall, just some small comments.

@steff456 steff456 marked this pull request as ready for review June 8, 2021 19:22
@steff456 steff456 requested a review from kgryte June 8, 2021 19:42
@rgommers rgommers mentioned this pull request Jun 15, 2021
Closed
leofang
leofang requested changes Jun 24, 2021
View reviewed changes
Copy link
Contributor

@leofang leofang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @steff456! Thanks for putting these together. Based on the recent discussion summarized in #159 (comment), could you remove all *fft2 APIs? Thanks.

leofang
leofang requested changes Jul 14, 2021
View reviewed changes
Copy link
Contributor

@leofang leofang left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for my late response @steff456...I just reviewed {i}fft and {i}fftn and left some comments. We should reorder the list so that each pair of transforms appears together (see my comment below). I'll try to resume tomorrow...😅

@rgommers rgommers mentioned this pull request Jul 14, 2021
Closed
@steff456
Copy link
Member Author

@leofang I already made the changes, please let me know if there's something else we can improve here :)

@leofang leofang mentioned this pull request Jul 22, 2021
Closed
leofang
leofang requested changes Jul 22, 2021
View reviewed changes
Copy link
Contributor

@leofang leofang left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @steff456! I have reviewed fft/ifft/fftn/ifftn/rfft/irfft/rfftn/irfftn. I think C2C transforms (fft/ifft/fftn/ifftn) are in good shape now, but more work is needed for R2C/C2R transforms as the behavior is a bit messy and confusing. I hope we could find another pair of eyes to go over these.

I will try to finish the rest of reviews (hfft, fftfreq, etc) asap...

asmeurer
asmeurer reviewed Jul 22, 2021
View reviewed changes
@kgryte
Copy link
Contributor

kgryte commented Aug 22, 2022

@steff456 The table looks correct. Thanks for putting that together.

@kgryte kgryte mentioned this pull request Sep 8, 2022
Closed
leofang
leofang requested changes Sep 11, 2022
View reviewed changes
Copy link
Contributor

@leofang leofang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for delay. I reviewed all functions but rfftn and irfftn, as there are still errors to be ironed out in the R2C/C2R(-like) transforms. I also have a few proposals to simplify the standard, see my comments below.

Note that the input/output data types should be taken care, as noted in previous rounds of review, including:

  1. input data type promotion when applicable (pending #478)
  2. reject inconsistent input data types in R2C/C2R transforms

@leofang
Copy link
Contributor

leofang commented Sep 11, 2022

Another note: As discussed in a prior meeting it is preferable to add a note that the output dtype will be consistent with the input dtype (ex: we don't always promote to complex128 as NumPy currently does even if the input is in single precision), but I think if we word it correctly when addressing #478 this need should be automatically resolved.

@leofang
Copy link
Contributor

leofang commented Oct 5, 2022

Hi @steff456 Do you have local changes that you have not pushed? If not, may I edit in your branch directly?

@steff456
Copy link
Member Author

steff456 commented Oct 5, 2022

Hi @leofang, go ahead and make changes! Thanks a lot 😬

@leofang leofang self-requested a review October 5, 2022 17:58
@leofang
Copy link
Contributor

leofang commented Oct 15, 2022 •
edited
Loading

@kgryte @rgommers @peterbell10 @toslunar @steff456 @oleksandr-pavlyk This should be ready now. Any chance any of you can take a look? Otherwise, I'll self-merge before the next Array API meeting (Oct 20).

@rgommers
Copy link
Member

Thanks @leofang! I had a quick browse, it looks like it is in good shape. I'll defer to @kgryte for a last in-depth look if he wants to.

toslunar
toslunar reviewed Oct 17, 2022
View reviewed changes
@leofang leofang self-assigned this Oct 17, 2022
steff456
steff456 commented Oct 17, 2022
View reviewed changes
Copy link
Member Author

@steff456 steff456 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Thanks a lot for helping me with this PR @leofang and @kgryte 🎉

@leofang
Copy link
Contributor

leofang commented Oct 20, 2022

Announced in the meeting, let's merge. We can follow up with additional PRs if needed.

@leofang leofang dismissed kgryte’s stale review October 20, 2022 17:14

All addressed.

@leofang leofang merged commit 8949817 into data-apis:main Oct 20, 2022
@honno honno mentioned this pull request Oct 9, 2023
Closed
@kgryte kgryte mentioned this pull request Jan 8, 2024
Merged
@rgommers rgommers added the topic: FFT Fast Fourier transforms. label Jan 11, 2024
kgryte added a commit that referenced this pull request Feb 8, 2024
@asmeurer @leofang @kgryte
Clarify specification guidance in FFT functions
937d3b8 
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asmeurer asmeurer asmeurer left review comments

@rgommers rgommers rgommers left review comments

@leofang leofang leofang approved these changes

@kgryte kgryte kgryte left review comments

+1 more reviewer

@toslunar toslunar toslunar left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@leofang leofang

@steff456 steff456

Labels
API extension Adds new functions or objects to the API. topic: FFT Fast Fourier transforms.
Projects
None yet
Milestone
v2022
Development

Successfully merging this pull request may close these issues.

RFC: specifying the shape of the transformed output in n-dimensional FFT APIs
6 participants
@steff456 @rgommers @kgryte @leofang @asmeurer @toslunar