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

Implmentation of R2C FFT #26

Open
wants to merge 44 commits into
base: main
Choose a base branch
from
Open

Implmentation of R2C FFT #26

wants to merge 44 commits into from

Conversation

smu160
Copy link
Member

@smu160 smu160 commented Apr 30, 2024

@calebzulawski Draft for #23

@smu160 smu160 self-assigned this May 13, 2024
@smu160 smu160 added the enhancement New feature or request label May 24, 2024
@smu160
Copy link
Member Author

smu160 commented May 29, 2024

Hi @calebzulawski,

We now have a working version of R2C FFT. This version is clearly sub-optimal as I made correctness the main priority. I'll measure and tune the implementation for now. In the interim, I recall you mentioned, inter alia, ideas about the public API and the use of traits.

I'm curious to hear your thoughts. Thank you!

Best,
Saveliy

@smu160 smu160 requested a review from Shnatsel May 29, 2024 15:45
src/fft.rs Outdated Show resolved Hide resolved
@smu160 smu160 requested a review from calebzulawski May 29, 2024 16:04
@calebzulawski
Copy link
Contributor

Looks good! I think starting with correctness is the way to go. Regarding traits, I think I will probably submit a PR in the future after this has been merged.

@codecov-commenter
Copy link

codecov-commenter commented May 30, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 99.53052% with 1 line in your changes missing coverage. Please review.

Project coverage is 99.33%. Comparing base (7048416) to head (d099647).

Files Patch % Lines
utilities/src/lib.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #26      +/-   ##
==========================================
+ Coverage   99.16%   99.33%   +0.16%     
==========================================
  Files           8        9       +1     
  Lines         841     1053     +212     
==========================================
+ Hits          834     1046     +212     
  Misses          7        7              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@smu160 smu160 marked this pull request as ready for review June 4, 2024 00:20
@smu160
Copy link
Member Author

smu160 commented Jun 14, 2024

Looks good! I think starting with correctness is the way to go. Regarding traits, I think I will probably submit a PR in the future after this has been merged.

Hi @calebzulawski,

I believe we're finally at a point where we can scrutinize the implementation, make necessary changes, and merge. Please feel free to point out any deficiencies and I can work on fixing them.

Thank you!!

src/fft.rs Outdated Show resolved Hide resolved
src/fft.rs Outdated Show resolved Hide resolved
src/fft.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants