Precomputed Lagrange #957
Precomputed Lagrange #957
Conversation
|
|
||
| /// This function uses the `LagrangeTable` to evaluate `polynomial` on the specified output "x coordinates" | ||
| /// the "y coordinates" of the evaluation are multiplied to `result` | ||
| pub fn mult_result_by_evaluation( |
There was a problem hiding this comment.
Does this need to be public? It looks like it assumes a particular form for result that a caller isn't obligated to guarantee.
There was a problem hiding this comment.
There are some parts in the DZKP (within the provers computation) where I want to compute q(x')*p(x') for some x coordinate x' so it is convenient do use this function and pass q(x') as result which is mutated to q(x')*p(x'). There is the more general function eval for the general case.
There was a problem hiding this comment.
I would prefer not to embed this logic into the Lagrange module. I understand it might even be slightly more efficient this way - but it feels like we are violating an important separation of concerns.
There was a problem hiding this comment.
I am fine with removing it since you seem to prefer it that way.
|
you seem to have added a flaky test, we need to fix it before we can check the coverage thread 'protocol::ipa_prf::malicious_security::lagrange::test::test_lagrange_cannonical_using_from' panicked at ipa-core/src/protocol/ipa_prf/malicious_security/lagrange.rs:209:5: |
|
The test failures were due to a debug assertion that had an error and should be fixed now. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #957 +/- ##
==========================================
- Coverage 89.64% 88.57% -1.08%
==========================================
Files 182 159 -23
Lines 25705 21668 -4037
==========================================
- Hits 23044 19192 -3852
+ Misses 2661 2476 -185 ☔ View full report in Codecov by Sentry. |
| /// The "x coordinates" of the input points are `x_0` to `x_(N-1)` are `F::ZERO` to `(N-1)*F::ONE`. | ||
| /// The `LagrangeTable` also specifies `M` "x coordinates" for the output points | ||
| /// The "x coordinates" of the output points are `x_N` to `x_(N+M-1)` are `N*F::ONE` to `(N+M-1)*F::ONE`. | ||
| pub struct LagrangeTable<F: Field, N: ArrayLength, M: ArrayLength> { |
There was a problem hiding this comment.
I am surprised that it declares that it works over fields and not prime fields only.
There was a problem hiding this comment.
Its more general this way. If you want to load something precomputed for a field for which we dont have invert implemented, e.g. from a file, you could. Invert is only needed to generate the denominator, otherwise we don't need inversion.
| table: GenericArray<GenericArray<F, N>, M>, | ||
| } | ||
|
|
||
| impl<F, N> LagrangeTable<F, N, U1> |
There was a problem hiding this comment.
why do we need this specialization?
There was a problem hiding this comment.
We can make it more general, however we won't need it. M is the amount of output x-coordinates and this implementation allows to generate a Lagrange table for a single output point using new(x_output, denominator). In the proofs, we need to evaluate the polynomials on a single x-coordinate r which is random. Otherwise we always evaluate polynomials at fixed x-coordinates N...N+M in which case we can just use the implementation of From(denominator) below
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure what is going on here... but it looks wrong....
There was a problem hiding this comment.
I just use the proptest macro here. I added a function since the formatting is not checked within macro calls.
| let polynomial_monomial_form = MonomialFormPolynomial { | ||
| coefficients: GenericArray::<TestField, U8>::from_array(input_points), | ||
| }; | ||
| // the canonical x coordinates are 0..15, the outputs use coordinates 8..15: |
There was a problem hiding this comment.
I think there is an error in this comment. I think @danielmasny means:
the canonical x coordinates are 0..7, the outputs use coordinates 8..15
| let x_coordinates_output = GenericArray::<_, U7>::generate(|i| { | ||
| TestField::try_from(u128::try_from(i).unwrap() + 8).unwrap() | ||
| }); |
There was a problem hiding this comment.
Honestly, I find the generics here(e.g. U7, U8) really confusing. I wish there was some better way.
There was a problem hiding this comment.
@danielmasny is out on paternity leave. How should we take this forward? Should I just land this PR as is and make a few minor changes in a follow up PR? That seems like the simplest approach. @martinthomson ?
|
Forwards is a good direction to move and this code is close. |
cleaned up version of Lagrange