Arrabiata: prepare constraints for cross-terms computation #2702

dannywillems · 2024-10-14T07:45:45Z

This pull request prepares the constraints to compute the cross-terms after the different columns and arguments have been computed. Challenges must be treated as a variable, and will be in a future PR.

Previous attempt: #2695

codecov · 2024-10-14T09:59:07Z

Codecov Report

Attention: Patch coverage is 96.50000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 72.60%. Comparing base (594f689) to head (4b0fd17).

Files with missing lines	Patch %	Lines
arrabiata/src/main.rs	0.00%	6 Missing ⚠️
arrabiata/src/columns.rs	83.33%	1 Missing ⚠️

Additional details and impacted files

@@                             Coverage Diff                             @@
##           dw/improve-from-variable-implementation    #2702      +/-   ##
===========================================================================
+ Coverage                                    72.54%   72.60%   +0.05%     
===========================================================================
  Files                                          247      247              
  Lines                                        57691    57955     +264     
===========================================================================
+ Hits                                         41853    42076     +223     
- Misses                                       15838    15879      +41

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

We do suppose evaluations of the public inputs will be given after the witness.

It does depend on maximum degree given in parameter. The probability is not negligible if 2 is given.

And use it to generate random monomials.

marcbeunardeau88 · 2024-10-29T08:50:34Z

mvpoly/src/lib.rs

+/// All polynomials are supposed to be given as multivariate polynomials of the
+/// same degree and the same number of variables, even if they do not have the
+/// same number of variables nor the same degree.


is this an invariant the caller needs to respect ?

marcbeunardeau88 · 2024-10-29T08:58:18Z

mvpoly/src/lib.rs

+    let mut cross_terms = HashMap::new();
+    polys.into_iter().enumerate().for_each(|(i, poly)| {
+        let cross_terms_poly: HashMap<usize, F> = poly.compute_cross_terms(&eval1, &eval2, u1, u2);
+        cross_terms_poly.into_iter().for_each(|(p, value)| {


I would change the name of the p variable

p is for power, which is the power of r.

marcbeunardeau88 · 2024-10-29T09:11:43Z

mvpoly/src/lib.rs

+    polys: Vec<T>,
+    combiner1: F,
+    combiner2: F,
+    eval1: [F; N],
+    eval2: [F; N],
+    u1: F,
+    u2: F,


I m not sure I understand what is computed here ?
the text mentioned combining the polys with one combiner, and we have two of them here.
I understand that the two combiners are the alpha from each instances we fold ?
But then I don't get the assymetrical role ?

See https://hackmd.io/@dannywillems/Syo5MBq90#Computation-of-cross-terms-for-shifted-polynomials

marcbeunardeau88 · 2024-10-29T09:18:01Z

arrabiata/tests/constraints.rs

+
+// This test is mostly an example to show to compute the cross-terms when we do
+// have the expressions, and some evaluations.
+// It doesn't test anything in particular. It is mostly an "integration" test.


A way to make this test test thing would be to checked the cross term property
poly(a +r*b) = poly(a) + r^n poly(b) + \sum_i r^i cross_term_i(a,b)
Also a nit: I wouldn't call that an integration test

"Also a nit: I wouldn't call that an integration test"

That's the reason "integration" is double quoted.

marcbeunardeau88 · 2024-10-29T09:20:18Z

mvpoly/src/lib.rs

            // Computing α'^i * value, and adding it to the next power of r
-            let alpha_i = combiner2.pow([i as u64]);
+            // Handling 0^0 = 1


isn't combiner a random term that cannot be zero ?

Yes, but it was monstly for the tests (see test directory). I didn't handle it initially. Even if the case is negligible, I think it is ok to have it. Maybe we should fail instead?

I'd ignore it to simplify the code, and remove the testing with zero, but do as you feel

marcbeunardeau88 · 2024-10-29T09:25:59Z

mvpoly/src/utils.rs

+/// It is useful when the number of nested loop is large and the size of the
+/// loops is large as well. `filter_map` is used to avoid computing the indices
+/// that are not needed and avoid a `capacity_overflow` error.
+pub fn compute_indices_nested_loop_upper_bound(


I would not expose this function, keep this implementation, and expose compute_indices_nested_loop implemented with that implementation, while computing the upper bound on the fly with a max.
Then we have one function instead of two, which is the efficient variant, and only need one argument

marcbeunardeau88 · 2024-10-29T09:29:17Z

I could not convinced my self of the correctness of the algorithm, and there is no test that reassure me.
Otherwise only small remarks.

marcbeunardeau88

I'd like a test on the defining property of cross terms to convince me of the correctness of the algorithm, which I wasn't able to check manually

dannywillems · 2024-10-29T10:30:08Z

mvpoly/src/lib.rs

+/// linearly combined using the power of a combiner, often called `α`.
+/// Powers of the combiner are considered as an additional variable of the
+/// multi-variate polynomials, therefore increasing the degree of the sum of all
+/// polynomials by one. In other words, if we combine (M + 1) polynomials


See the comment just above:

/// Powers of the combiner are considered as an additional variable of the /// multi-variate polynomials, therefore increasing the degree of the sum of all /// polynomials by one.

Instead of having a power, we do consider them as a different variable.

dannywillems · 2024-10-29T10:30:28Z

mvpoly/src/lib.rs

+/// cross-terms of the polynomial:
+///
+/// ```text
+/// P(X_1, ..., X_N, α_1, ..., α_M) =     P_1(X_1, ..., X_N)


See https://github.com/o1-labs/proof-systems/pull/2702/files#r1820537584

dannywillems · 2024-10-29T10:35:57Z

I could not convinced my self of the correctness of the algorithm, and there is no test that reassure me.

See https://hackmd.io/@dannywillems/Syo5MBq90#Computation-of-cross-terms-for-shifted-polynomials. And we also discussed it in the past in person. Agreed to have more comments and more PBT.

dannywillems self-assigned this Oct 14, 2024

dannywillems marked this pull request as draft October 14, 2024 07:45

Base automatically changed from dw/remove-old-methods-in-interpreter to master October 14, 2024 15:57

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch 5 times, most recently from 4f07a8a to f713363 Compare October 15, 2024 06:56

dannywillems changed the base branch from master to dw/mvpoly-from-expr-generic-over-challenge October 15, 2024 08:26

dannywillems mentioned this pull request Oct 15, 2024

Arrabiata: WIP #2695

Closed

dannywillems force-pushed the dw/mvpoly-from-expr-generic-over-challenge branch from a35fa79 to 0ef15d6 Compare October 15, 2024 08:29

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch 4 times, most recently from 70addbd to 0129e25 Compare October 15, 2024 14:29

dannywillems mentioned this pull request Oct 15, 2024

MVPoly: improve the library + fix sub in monomials #2713

Merged

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch from 0129e25 to 32a4f93 Compare October 15, 2024 14:39

dannywillems changed the base branch from dw/mvpoly-from-expr-generic-over-challenge to dw/mvpoly-misc-changes October 15, 2024 14:39

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch from 32a4f93 to 97fbed5 Compare October 15, 2024 14:56

Base automatically changed from dw/mvpoly-misc-changes to master October 15, 2024 16:27

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch from 97fbed5 to 9eff145 Compare October 15, 2024 16:56

dannywillems changed the base branch from master to dw/improve-from-variable-implementation October 15, 2024 16:58

dannywillems force-pushed the dw/improve-from-variable-implementation branch 3 times, most recently from 0b6dd27 to 594f689 Compare October 16, 2024 12:52

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch 2 times, most recently from 6b94b8b to bb75668 Compare October 16, 2024 13:17

dannywillems requested a review from marcbeunardeau88 October 16, 2024 13:19

dannywillems marked this pull request as ready for review October 16, 2024 13:20

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch from 9f866ce to 9b7b501 Compare October 16, 2024 21:57

dannywillems marked this pull request as ready for review October 16, 2024 21:57

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch from 9b7b501 to 4b0fd17 Compare October 16, 2024 22:01

Base automatically changed from dw/improve-from-variable-implementation to master October 17, 2024 13:54

dannywillems added 5 commits October 28, 2024 15:44

MVPoly: cross-terms computation of a combili of mvpoly

c28b4d4

Arrabiata/main: add more future tasks to remind

3c2334b

Arrabiata/Column: implement column to usize

3864abb

Arrabiata/Column: support conversion of public inputs into usize

687e400

We do suppose evaluations of the public inputs will be given after the witness.

Arrabiata/Constraints: integrate mvpoly to compute cross-terms

1e9e66c

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch from 4b0fd17 to 1e9e66c Compare October 28, 2024 14:44

dannywillems added 8 commits October 28, 2024 15:45

Arrabiata/Column: support conversion of public inputs into usize

d60a681

We do suppose evaluations of the public inputs will be given after the witness.

MVPoly: method to compute the cross terms of a list of polynomials

d3fe0ba

Arrabiata/Constraints: integrate mvpoly to compute cross-terms

cf76131

Arrabiata/tests: remove old comment

28b4d66

MVPoly/monomials: improve generation of random generation

98cdfd9

MVPoly/PBT: remove a flaky test for multilinearity

44a3f08

It does depend on maximum degree given in parameter. The probability is not negligible if 2 is given.

MVPoly/utils: compute nested loop indices with an upper bound

ba0fad3

And use it to generate random monomials.

MVPoly/Cross-terms: add PBT

c23758f

dannywillems force-pushed the dw/mvpoly-arrabiata-convert-into branch from 7e956d0 to c23758f Compare October 28, 2024 15:03

marcbeunardeau88 reviewed Oct 29, 2024

View reviewed changes

marcbeunardeau88 requested changes Oct 29, 2024

View reviewed changes

dannywillems commented Oct 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arrabiata: prepare constraints for cross-terms computation #2702

Arrabiata: prepare constraints for cross-terms computation #2702

dannywillems commented Oct 14, 2024 •

edited

Loading

codecov bot commented Oct 14, 2024 •

edited

Loading

marcbeunardeau88 Oct 29, 2024

marcbeunardeau88 Oct 29, 2024

dannywillems Oct 29, 2024

marcbeunardeau88 Oct 29, 2024

dannywillems Oct 29, 2024

marcbeunardeau88 Oct 29, 2024

dannywillems Oct 29, 2024 •

edited

Loading

marcbeunardeau88 Oct 29, 2024

dannywillems Oct 29, 2024

marcbeunardeau88 Oct 29, 2024

marcbeunardeau88 Oct 29, 2024

marcbeunardeau88 commented Oct 29, 2024

marcbeunardeau88 left a comment

dannywillems Oct 29, 2024

dannywillems Oct 29, 2024

dannywillems commented Oct 29, 2024 •

edited

Loading

Arrabiata: prepare constraints for cross-terms computation #2702

Are you sure you want to change the base?

Arrabiata: prepare constraints for cross-terms computation #2702

Conversation

dannywillems commented Oct 14, 2024 • edited Loading

codecov bot commented Oct 14, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannywillems Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcbeunardeau88 commented Oct 29, 2024

marcbeunardeau88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannywillems commented Oct 29, 2024 • edited Loading

dannywillems commented Oct 14, 2024 •

edited

Loading

codecov bot commented Oct 14, 2024 •

edited

Loading

dannywillems Oct 29, 2024 •

edited

Loading

dannywillems commented Oct 29, 2024 •

edited

Loading