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

Make Babybear field FFT friendly #576

Merged
merged 32 commits into from
Dec 7, 2023

Conversation

mdvillagra
Copy link
Contributor

@mdvillagra mdvillagra commented Sep 25, 2023

Make Babybear field FFT friendly

Description

Implemented isFFTtrait to babybear field as requested in issue #539. Here the constantsTWO_ADIC_PRIMITVE_ROOT_OF_UNITY and TWO_ADICITY were updated accordingly.

Type of change

  • New feature

Checklist

  • Linked to Github Issue
  • Unit tests added

@mdvillagra mdvillagra requested review from schouhy, ajgara and a team as code owners September 25, 2023 20:09
@mdvillagra mdvillagra marked this pull request as draft September 25, 2023 22:36
@mdvillagra mdvillagra marked this pull request as ready for review September 27, 2023 12:29
@mdvillagra
Copy link
Contributor Author

Now all fft tests work but the CI / Test (macOS, Apple sillicon) check is not passing.

@MauroToscano
Copy link
Collaborator

There's a bug with how the CUDA and Metal is used. It's assuming that if the feature is enabled, it can run with CUDA or Metal. As a quickfix, you can add this in the relevant tests#[cfg(not(feature = "instruments"))]. We should fix this in more general way, even if the feature is not enabled it could backoff to the CPU version, but it's not critical.

@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2023

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (0f3b5ef) 96.16% compared to head (9e26c4c) 96.14%.
Report is 11 commits behind head on main.

Files Patch % Lines
math/src/field/fields/fft_friendly/babybear.rs 92.69% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #576      +/-   ##
==========================================
- Coverage   96.16%   96.14%   -0.03%     
==========================================
  Files         132      133       +1     
  Lines       29904    29812      -92     
==========================================
- Hits        28758    28662      -96     
- Misses       1146     1150       +4     

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

@diegokingston diegokingston added this pull request to the merge queue Dec 7, 2023
Merged via the queue into lambdaclass:main with commit 438113f Dec 7, 2023
7 checks passed
@mdvillagra mdvillagra deleted the babybear-isfft-#539 branch December 18, 2023 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants