Add Binary Field #984
Add Binary Field #984
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #984 +/- ##
==========================================
+ Coverage 72.00% 72.12% +0.11%
==========================================
Files 159 160 +1
Lines 34867 35200 +333
==========================================
+ Hits 25107 25388 +281
- Misses 9760 9812 +52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// Joins the hi and low part making a new element of a bigger level. | ||
| /// For example, if a_hi = x and a_low = 1 | ||
| /// then a = xy + 1. | ||
| pub fn join(&self, low: &Self) -> Self { |
There was a problem hiding this comment.
at some point, we could need something like pack (join many)
| // Equality check | ||
| pub fn equals(&self, other: &Self) -> bool { | ||
| self.value == other.value() | ||
| } |
There was a problem hiding this comment.
Just in case, did you consider a custom implementation of Eq instead? If you did, could you add a comment explaining why you opted for a method instead?
| let high = self.value() >> other.num_bits(); | ||
| // We concatenate both parts. | ||
| let result_value = (high << other.num_bits()) | result_low; |
There was a problem hiding this comment.
I'm a bit confused about this. Wouldn't the high bits of other be 0 already, making this essentially the same as XOR of both?
There was a problem hiding this comment.
You are totally right, thanks for the comment! We fixed it here: 251812a and now the function looks much better.
Oppen
left a comment
Oppen
left a comment
There was a problem hiding this comment.
I leave this comment as a reminder as what is missing review from me (I need a fresher brain to look at those carefully):
- mul
- inv
- tests
Everything not in that list I just reviewed and looks quite well.
| impl Default for TowerFieldElement { | ||
| fn default() -> Self { | ||
| Self::new(0, 0) | ||
| } | ||
| } |
There was a problem hiding this comment.
This can be easily implemented deriving Default in the struct
There was a problem hiding this comment.
Not necessarily now, but the following would be desirable for benches:
- A iai-callgrind one that runs on the CI (we already have a job set up for that);
- Comparisons against a different implementation.
crates/math/benches/fields/binary.rs
Outdated
| let mut group = c.benchmark_group("Binary TowerField add"); | ||
| let a = rand_element(num_levels_a); | ||
| let b = rand_element(num_levels_b); |
There was a problem hiding this comment.
We should probably use vectors of random elements instead, so we don't accidentally pick a biased pair that performs much better or much worse than average, for example.
| // Step 3: Compute high_product * x_{n-1} | ||
| let shifted_high_product = high_product.mul(x_value); |
There was a problem hiding this comment.
This will probably perform better with a straight shift. That way we avoid not just the multiplications, but the recursion.
There was a problem hiding this comment.
Don't change it though. We'll see eventually if this is a bottleneck or not.
| } | ||
|
|
||
| /// Calculate power. | ||
| pub fn pow(&self, exp: u32) -> Self { |
There was a problem hiding this comment.
Love this, pretty and concise.
| impl<'a> Mul<&'a TowerFieldElement> for &'a TowerFieldElement { | ||
| type Output = TowerFieldElement; | ||
|
|
||
| fn mul(self, other: &'a TowerFieldElement) -> TowerFieldElement { | ||
| <TowerFieldElement as Mul<TowerFieldElement>>::mul(*self, *other) | ||
| } | ||
| } |
There was a problem hiding this comment.
I believe you can omit the lifetimes here, as the reference never outlives the call.
If it asks for impl<'a>, impl <'_> might work instead.
| let a = TowerFieldElement::new(00, 1); // 0 | ||
| let b = TowerFieldElement::new(01, 1); // 1 | ||
| let c = TowerFieldElement::new(10, 1); // x | ||
| let d = TowerFieldElement::new(11, 1); // x + 1 |
There was a problem hiding this comment.
Maybe you want 0b00 through 0b11 here. The clippy lint was actually warning correctly if that's the case.
…ss/lambdaworks into add_binary_field_tower
…aworks into add_binary_field_tower
Add Binary Field
Description
This PR introduces binary fields and extends them using a tower-based approach.
It implements arithmetic in GF(2^(2^n)) by representing each field element as a polynomial over GF(2)
We didn't implement IsField or FieldElement in this case because we wanted elements from different field extensions to coexist within the same struct, allowing us to optimize each operation to work simultaneously across all levels.
Operations implemented:
Addition: Implemented as bitwise XOR.
Multiplication: Performed recursively using Karatsuba optimization.
Inversion: Achieved via recursive methods and Fermat's little theorem.
Exponentiation: Implemented using the square-and-multiply algorithm.
Type of change
Please delete options that are not relevant.
Checklist