Add Babybear field #549

mdvillagra · 2023-09-05T21:53:30Z

Babybear field implementation

Description

Add Babybear field with arithmetic operations as requested on issue #539. The field uses MontgomeryBackendPrimeField and implements the same traits as in Stark252PrimeField. The field elements are encoded using type u64 considering that Babybear only requires 31 bits.

Type of change

New feature
Bug fix
Optimization

Checklist

Linked to Github Issue
Unit tests added
This change requires new documentation.
- Documentation has been added/updated.
This change is an Optimization
- Benchmarks added/run

… main

codecov-commenter · 2023-09-05T21:58:18Z

Codecov Report

Merging #549 (442dabd) into main (29af640) will decrease coverage by 0.04%.
The diff coverage is 88.13%.

@@            Coverage Diff             @@
##             main     #549      +/-   ##
==========================================
- Coverage   95.83%   95.79%   -0.04%     
==========================================
  Files          78       79       +1     
  Lines       12311    12370      +59     
==========================================
+ Hits        11798    11850      +52     
- Misses        513      520       +7

Files Changed	Coverage Δ
math/src/unsigned_integer/element.rs	`97.17% <ø> (ø)`
math/src/field/fields/fft_friendly/babybear.rs	`88.13% <88.13%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

MauroToscano

Babybear Field should be using the MontgomeryBackendPrimeField. The code should set the right constants using that backend. See Stark252PrimeFieldas an example

mdvillagra · 2023-09-07T01:23:17Z

Babybear Field should be using the MontgomeryBackendPrimeField. The code should set the right constants using that backend. See Stark252PrimeFieldas an example

What about the field operations? Do I still leave them there? Or just implement the modulus? The file Stark252PrimeField doesn't have any field operations implemented.

schouhy

Nice! Thanks for the PR!
I left a minor request to move and rename stuff.

schouhy · 2023-09-08T18:12:17Z

math/src/field/fields/fft_friendly/babybear.rs

+    unsigned_integer::element::UnsignedInteger,
+};
+
+pub type U64 = UnsignedInteger<1>;


Please move this to unsigned_integer/element.rs (here). So all unsigned integer type aliases are together.

This a nice idea

schouhy · 2023-09-08T18:21:20Z

math/src/field/fields/fft_friendly/babybear.rs

+};
+
+pub type U64 = UnsignedInteger<1>;
+pub type U64PrimeField<T> = MontgomeryBackendPrimeField<T, 1>;


There's already a U64PrimeField (here).
@MauroToscano @Oppen I guess there's no need to have both u64primefields if the one with the Montgomery backend is more performant. Maybe we should create an issue to benchmark them and remove the slow one.
For the time being I would change the name of this type alias to U64MontgomeryBackendPrimeField or something along those lines. To avoid confusion.

We should review this. But let's do it ourselves, and discuss it outside this PR.

The change of name would be nice though

mdvillagra · 2023-09-08T23:22:01Z

guys, I don't know why another PR was created about this issue. I just did a push and that's it.

schouhy · 2023-09-08T23:23:23Z

no worries. Looks like they point to different branches in your fork.

Mdvillagra/issue539

mdvillagra · 2023-09-08T23:43:23Z

I did some clean up on my fork, and now it seems to be fine. I didn't noticed but I was working on another branch. Now I pushed into my main branch.

schouhy · 2023-09-08T23:46:30Z

Cool. Looks like the lint check is failing. That's prolly solved by running cargo fmt.

schouhy

Awesome. Thanks!

MauroToscano

Since there's code for the FFT/NTT, there should be a test to check if it's working properly. It can be submitted as a package of IsFFt with tests in another PR, or fixed here. The IsFieldpart seems right.

Either add the tests, or remove the FFT part

mdvillagra · 2023-09-11T16:29:31Z

Since there's code for the FFT/NTT, there should be a test to check if it's working properly. It can be submitted as a package of IsFFt with tests in another PR, or fixed here. The IsFieldpart seems right.

Either add the tests, or remove the FFT part

I will delete the FFT part right now and add it in another PR.

dbejarano820 · 2023-09-13T02:12:19Z

.vscode/settings.json

+{
+    "githubPullRequests.ignoredPullRequestBranches": [
+        "main"
+    ]
+}


Do we really want to merge this?

should probably add it to the .gitignore

probably is better to just delete it right?

I deleted the file

MauroToscano · 2023-09-13T17:48:24Z

math/src/field/fields/fft_friendly/babybear.rs

+impl PartialOrd for FieldElement<Babybear31PrimeField> {
+    fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
+        self.representative().partial_cmp(&other.representative())
+    }
+}
+
+impl Ord for FieldElement<Babybear31PrimeField> {
+    fn cmp(&self, other: &Self) -> core::cmp::Ordering {
+        self.representative().cmp(&other.representative())
+    }
+}


This isn't needed, but it's ok, we can document this later and remove if needed. Doesn't hurt either

mdvillagra added 4 commits September 5, 2023 09:45

starting with babybear field and add function

8c24af3

implemented all traits for babybear

e008b4b

added test for the babybear field

8233c08

Merge branch 'main' of https://github.com/mdvillagra/lambdaworks into…

6f32048

… main

mdvillagra requested review from schouhy, ajgara and a team as code owners September 5, 2023 21:53

MauroToscano requested changes Sep 6, 2023

View reviewed changes

mdvillagra and others added 2 commits September 7, 2023 12:36

Merge branch 'lambdaclass:main' into main

9c2d333

Babybear field from MontgomeryBackendPrimeField

c260508

mdvillagra requested a review from MauroToscano September 7, 2023 19:01

added tests to babybear.rs

fbb79e8

mdvillagra changed the title ~~Added Babybear field with arithmetic operations~~ Add Babybear field Sep 8, 2023

schouhy suggested changes Sep 8, 2023

View reviewed changes

mdvillagra and others added 3 commits September 8, 2023 19:08

added requested changes and isFFTField trait

b180b79

Merge branch 'lambdaclass:main' into main

80fbf1d

Merge branch 'main' into mdvillagra/issue539

19f209e

Merge pull request #1 from mdvillagra/mdvillagra/issue539

ba436e6

Mdvillagra/issue539

ran cargo fmt to pass the lint check.

2132bbb

schouhy approved these changes Sep 8, 2023

View reviewed changes

diegokingston approved these changes Sep 9, 2023

View reviewed changes

MauroToscano requested changes Sep 11, 2023

View reviewed changes

deleted the isFFTField trait implementation

315ddfb

mdvillagra requested a review from MauroToscano September 11, 2023 16:39

dbejarano820 reviewed Sep 13, 2023

View reviewed changes

deleted .vscode directory

442dabd

MauroToscano reviewed Sep 13, 2023

View reviewed changes

MauroToscano approved these changes Sep 13, 2023

View reviewed changes

dbejarano820 approved these changes Sep 13, 2023

View reviewed changes

diegokingston added this pull request to the merge queue Sep 15, 2023

Merged via the queue into lambdaclass:main with commit b394a6a Sep 15, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Babybear field #549

Add Babybear field #549

mdvillagra commented Sep 5, 2023 •

edited

Loading

codecov-commenter commented Sep 5, 2023 •

edited

Loading

MauroToscano left a comment •

edited

Loading

mdvillagra commented Sep 7, 2023

schouhy left a comment •

edited

Loading

schouhy Sep 8, 2023

MauroToscano Sep 8, 2023

schouhy Sep 8, 2023 •

edited

Loading

MauroToscano Sep 8, 2023

MauroToscano Sep 8, 2023

mdvillagra commented Sep 8, 2023 •

edited

Loading

schouhy commented Sep 8, 2023 •

edited

Loading

mdvillagra commented Sep 8, 2023

schouhy commented Sep 8, 2023

schouhy left a comment

MauroToscano left a comment •

edited

Loading

mdvillagra commented Sep 11, 2023

dbejarano820 Sep 13, 2023

dbejarano820 Sep 13, 2023

mdvillagra Sep 13, 2023

mdvillagra Sep 13, 2023

MauroToscano Sep 13, 2023

Add Babybear field #549

Add Babybear field #549

Conversation

mdvillagra commented Sep 5, 2023 • edited Loading

Babybear field implementation

Description

Type of change

Checklist

codecov-commenter commented Sep 5, 2023 • edited Loading

Codecov Report

MauroToscano left a comment • edited Loading

Choose a reason for hiding this comment

mdvillagra commented Sep 7, 2023

schouhy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schouhy Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdvillagra commented Sep 8, 2023 • edited Loading

schouhy commented Sep 8, 2023 • edited Loading

mdvillagra commented Sep 8, 2023

schouhy commented Sep 8, 2023

schouhy left a comment

Choose a reason for hiding this comment

MauroToscano left a comment • edited Loading

Choose a reason for hiding this comment

mdvillagra commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdvillagra commented Sep 5, 2023 •

edited

Loading

codecov-commenter commented Sep 5, 2023 •

edited

Loading

MauroToscano left a comment •

edited

Loading

schouhy left a comment •

edited

Loading

schouhy Sep 8, 2023 •

edited

Loading

mdvillagra commented Sep 8, 2023 •

edited

Loading

schouhy commented Sep 8, 2023 •

edited

Loading

MauroToscano left a comment •

edited

Loading