-
Notifications
You must be signed in to change notification settings - Fork 16
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
BaseFold: all open functions accept Arc
instead of DenseMultilinearExtension
#563
base: master
Are you sure you want to change the base?
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.
The unification looks like a reasonable thing to do.
Though I wonder if it's possible to break the PR into even more manageable chunks? (It's good you broke this one out at all, it's already pretty beefy on its own!)
mpcs/benches/basefold.rs
Outdated
@@ -143,10 +144,14 @@ fn bench_batch_commit_open_verify_goldilocks<Pcs: PolynomialCommitmentScheme<E>> | |||
let values: Vec<E> = evals | |||
.iter() | |||
.map(Evaluation::value) | |||
.map(|x| *x) | |||
.copied() |
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.
That's an unrelated improvement, isn't it? (It's small enough that we can leave it in this PR, however.)
mpcs/benches/basefold.rs
Outdated
@@ -182,7 +187,7 @@ fn bench_batch_commit_open_verify_goldilocks<Pcs: PolynomialCommitmentScheme<E>> | |||
let values: Vec<E> = evals | |||
.iter() | |||
.map(Evaluation::value) | |||
.map(|x| *x) | |||
.copied() | |||
.collect::<Vec<E>>(); |
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.
(Not part of your PR: if we specify the type in line 187 (on the let
), we don't need to also specify it here, too.)
Extracting from #294
Waiting for #554 to merge.
Currently, only
simple_batch_open
is requiringArc
for polys in API while other open functions are acceptingDenseMultilinearExtension
. This PR unifies them.