-
Notifications
You must be signed in to change notification settings - Fork 256
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
Implemented add, mult, inv, sub, div, pow and neg for binary fields and wrote tests for each function #864
base: master
Are you sure you want to change the base?
Conversation
…nd wrote tests for each function
} | ||
|
||
// Method for addition in GF(2^k) | ||
pub fn add(&self, other: &Self) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here why not implementing directly the Add
trait like this is done here for example? I think this is also applicable to sub, mul, neg... so that we can use directly +
, -
, *
operations
Lines 192 to 200 in 9ce33e6
impl<'a, P: Pairing> Add<&'a Self> for PairingOutput<P> { | |
type Output = Self; | |
#[inline] | |
fn add(mut self, other: &'a Self) -> Self { | |
self += other; | |
self | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do this! Thanks for the suggestion
} | ||
|
||
// Binary field multiplication with reduction | ||
fn binmul(v1: BigUint, v2: BigUint, length: Option<usize>) -> BigUint { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can integrate this method directly inside the BinaryFieldElement
impl no? using:
fn binmul(&self, rhs: Self, length: Option<usize>) -> Self {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SO we could, but why would someone use binmul instead of mul? I think mul is more intuitive right?
|
||
|
||
#[cfg(test)] | ||
mod tests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add some randomized tests here as this is done here for example? https://github.com/arkworks-rs/algebra/blob/master/ff/src/biginteger/tests.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done! Thanks for the suggestion
ff/src/fields/bf.rs
Outdated
#[derive( Clone, PartialEq, Eq)] | ||
pub struct BinaryFieldElement { | ||
value: BigUint, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I'm not sure this way of doing things is the more efficient approach because no matter the size of the field concerned, you store in your value
as BigUint
a vector of BigDigit
(u64 or 32 I think) but for field extensions before 32, we don't need as much space.
So I wonder if it is not more optimized to do something custom made like this:
so that we can have a very efficient implementation for small extensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes a lot of sense, but it would mean implementing a separate Binary Field implementation for each field size. Should I submit these changes as part of my next PR, or should I first push the other changes we discussed (operators and moving binmul) and then implement the different field sizes in subsequent PRs?
create a clickable link to the BinaryFieldElement type definition Co-authored-by: Thomas Coratger <60488569+tcoratger@users.noreply.github.com>
Co-authored-by: Thomas Coratger <60488569+tcoratger@users.noreply.github.com>
Co-authored-by: Thomas Coratger <60488569+tcoratger@users.noreply.github.com>
Co-authored-by: Thomas Coratger <60488569+tcoratger@users.noreply.github.com>
Only file to review is bf.rs inside /algebra/ff/src/fields
Description:
Implemented add, mult, inv, sub, div, pow and neg for binary fields and wrote tests for each function.
Description
closes: #XXXX
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Pending
section inCHANGELOG.md
Files changed
in the GitHub PR explorer