-
Notifications
You must be signed in to change notification settings - Fork 18
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
Refactor BaseFold test code #552
Conversation
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.
Looks ok in general, but please have a look at my comments. Thanks!
mpcs/src/basefold.rs
Outdated
// Both challenge and poly are over base field | ||
run_batch_commit_open_verify::<GoldilocksExt2, PcsGoldilocksRSCode>(true, 10, 11); | ||
fn commit_open_verify_goldilocks() { | ||
for base in [true, false].into_iter() { |
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.
Small cosmetic suggestion:
for base in [true, false].into_iter() { | |
for base in [true, false] { |
(Similar for the other loops, too.)
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.
@yczhangsjtu good suggestion to pick
|
||
pub fn gen_rand_poly<E: ExtensionField>( | ||
num_vars: usize, | ||
base: bool, |
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.
Didn't we have a PR where we moved away from using booleans here (and in similar situations)?
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.
Hmm, I guess that's happening in the follow up PR?
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.
@yczhangsjtu feel free either to resolve coversation (so i can merge) or change some code.
Extracting small PR from #294. The two benchmarks `benches_commit_verify_rs` and `benches_commit_verify_basecode` are similar, differing only in the choice of parameter. Merge them into one benchmark file. Also simplify the codes using the test utility functions introduced in #552 Waiting for #552 to merge. --------- Co-authored-by: Matthias Görgens <[email protected]>
Extracting small PR from #294
Refactor the test code in
mpcs/src/lib.rs
. Define some utility functions to simplify the tests.Also merged the test functions in
mpcs/src/basefold.rs
.