-
Notifications
You must be signed in to change notification settings - Fork 28
support build virtual polynomials in expression style #937
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
support build virtual polynomials in expression style #937
Conversation
…sion_based_virtualpoly
|
||
impl<E: ExtensionField> Expression<E> { | ||
pub(super) fn to_monomial_form_inner(&self) -> Self { | ||
Self::combine(self.distribute()).into_iter().sum() | ||
pub fn get_monomial_terms(&self) -> Vec<Term<Expression<E>, Expression<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.
just renaming since "to_xxxx" usually means ownership change, thus get_xxxx
more properly
@@ -895,31 +898,8 @@ impl<'a, E: ExtensionField> MultilinearExtension<E> for RangedMultilinearExtensi | |||
unimplemented!() | |||
} | |||
|
|||
fn fix_high_variables(&self, partial_point: &[E]) -> Self::Output { |
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 fix_high_variables
implementation is unused and actually its wrong, for we need to consider (start,end) due to it's range poly. Thus here clean it up and mark unimplemented!()
products_sum += AdditiveVec(sum); | ||
exit_span!(span); | ||
products_sum | ||
|mut uni_polys, (_half_eq_opt, MonomialTerms { terms })| { |
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.
No logic change here. Just rename
'products_sum' -> 'uni_polys'
'coefficient' -> scalar
products_sum += AdditiveVec(sum); | ||
exit_span!(span); | ||
products_sum | ||
|mut uni_polys, (_half_eq_opt, MonomialTerms { terms })| { |
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.
same as above
No logic change here. Just rename
'products_sum' -> 'uni_polys'
'coefficient' -> scalar
@@ -1004,7 +1051,7 @@ pub mod fmt { | |||
if only_one_limb { | |||
data[0].to_string() | |||
} else { | |||
format!("{name}[{}]", data.join(",")) | |||
format!("[{}]", data.join(",")) |
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.
we do not need original naming, due to plonky3 field already implement better formatter. This logic is only serve old goldilocks package
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.
LGTM!
To close #936
Design rationales
VirtualPolynomialsBuilder
to lift a witness of "ArcPoly" type to expression container, so they can involve into expression domain for calculationVirtualPolynomialsBuilder
in tower prover.Either<Base, Ext>
typeVirtualPolynomialsBuilder
is more like a util function for ceno main sumcheck flow.For GKR layer circuit in gk- iop #799 , the expression system will directly applied on chip-builder and skip
VirtualPolynomialsBuilder
benchmark
there is no impact for e2e benchmark before/after this change, which is expected
2^20
2^21
2^22