LogUp-GKR evaluator#18
Conversation
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Thank you! This is not a full review - I just looked at the math crate-related changes and left some comments.
Overall, I think we should try to implement this in phases. For example, the first PR in Winterfell could be just for introducing new functions/modules into the math crate. When we do that, we take the time to implement most things in near-final form (e.g., removing un-needed allocations, potentially parallelizing the most critical things etc.)
| /// This means that we can compute the evaluations of q at 1, ..., n - 1 using the evaluations | ||
| /// of p and thus reduce by 1 the size of the interpolation problem. | ||
| /// Once the coefficient of q are recovered, the c0 coefficient is appended to these and this | ||
| /// is precisely the coefficient representation of the original polynomial q. |
There was a problem hiding this comment.
What does q refer to here?
There was a problem hiding this comment.
q is defined in the two bulletpoints just above, it is just the polynomial resulting from factoring x.
| // construct the vector of interpolation points 1, ..., n | ||
| let n_minus_1 = self.partial_evaluations.len(); | ||
| let mut points = vec![E::BaseField::ZERO; n_minus_1]; | ||
|
|
||
| // construct their inverses. These will be needed for computing the evaluations | ||
| // of the q polynomial as well as for doing the interpolation on q | ||
| points | ||
| .iter_mut() | ||
| .enumerate() | ||
| .for_each(|(i, node)| *node = E::BaseField::from(i as u32 + 1)); |
There was a problem hiding this comment.
This is just constructing a vector with values 1, 2, 3, 4 ... - right? If so, we could do this as something like this to avoid first allocating and zeroing out the vector and then filling it in with values:
let points = (1..n_minus_1).iter().map(E::BaseField::from).collect::<Vec<_>>();| /// ```ignore | ||
| /// EQ: {0 , 1}^ν ⛌ {0 , 1}^ν ⇾ {0 , 1} | ||
| /// ((x_0, ..., x_{ν - 1}), (y_0, ..., y_{ν - 1})) ↦ \prod_{i = 0}^{ν - 1} (x_i * y_i + (1 - x_i) | ||
| /// * (1 - y_i)) | ||
| /// ``` |
There was a problem hiding this comment.
These, and other formulas, we should explicitly write as formals (i.e., using $$ in comments) so that they are displayed nicely in the docs.
There was a problem hiding this comment.
Added but not sure if it is the right way to do it. Do you have some documentation to help?
| /// Computes the evaluations of the Lagrange basis polynomials over the interpolating | ||
| /// set {0 , 1}^ν at (r_0, ..., r_{ν - 1}) i.e., the Lagrange kernel at (r_0, ..., r_{ν - 1}). | ||
| fn compute_lagrange_basis_evals_at<E: FieldElement>(query: &[E]) -> Vec<E> { |
There was a problem hiding this comment.
I haven't thought about this too much, but I'm curious if there is a way to parallelize this function.
There was a problem hiding this comment.
Thought about it a bit but couldn't come up with something clever. This would actually be a very important function to parallelize so we should give it more thought in my opinion. Will think more about it.
| /// Constructs a [`MultiLinearPoly`] from its evaluations over the boolean hyper-cube {0 , 1}^ν. | ||
| pub fn from_evaluations(evaluations: Vec<E>) -> Result<Self, MultiLinearPolyError> { | ||
| if !evaluations.len().is_power_of_two() { | ||
| return Err(MultiLinearPolyError::EvaluationsNotPowerOfTwo); | ||
| } | ||
| Ok(Self { | ||
| num_variables: (evaluations.len().ilog2()) as usize, | ||
| evaluations, | ||
| }) | ||
| } |
There was a problem hiding this comment.
I think it is OK to panic here is the number of evaluations is not a power of two. This way, we also won't need the MultiLinearPolyError enum.
| /// Computes f(r_0, y_1, ..., y_{ν - 1}) using the linear interpolation formula | ||
| /// (1 - r_0) * f(0, y_1, ..., y_{ν - 1}) + r_0 * f(1, y_1, ..., y_{ν - 1}) and assigns | ||
| /// the resulting multi-linear, defined over a domain of half the size, to `self`. | ||
| pub fn bind_least_significant_variable(&mut self, round_challenge: E) { | ||
| let mut result = vec![E::ZERO; 1 << (self.num_variables() - 1)]; | ||
| for (i, res) in result.iter_mut().enumerate() { | ||
| *res = self.evaluations[i << 1] | ||
| + round_challenge * (self.evaluations[(i << 1) + 1] - self.evaluations[i << 1]); | ||
| } | ||
| *self = Self::from_evaluations(result) | ||
| .expect("should not fail given that it is a multi-linear"); | ||
| } |
There was a problem hiding this comment.
I find this function a little confusing. I think what we are doing is reducing the number of variables by 1 and as the result, halving the number of evaluations - right?
If so, we should probably just return a new MultiLinearPoly from here. For example, the signature could be:
pub fn bind_least_significant_variable(self, variable_value: E) -> SelfAnother thing, it seems to me that we don't actually need to allocate the result vector. Instead, we may be able to update the self.evaluations vector in place and then just truncate its length. Or am I missing something?
There was a problem hiding this comment.
Are the two suggestions compatible? I went with the second one and removed the need for result
2 participants
Incomplete implementation of the proposal here
Notes: