Use a single implementation for combine_table_entry
#433
Conversation
| joint_combiner: F, | ||
| v: I, | ||
| ) -> F { | ||
| v.rev().fold(F::zero(), |acc, x| joint_combiner * acc + x) | ||
| v.rev() | ||
| .fold(F::zero(), |acc, x| joint_combiner.clone() * acc + x.clone()) |
There was a problem hiding this comment.
if you add Copy instead of Clone you can remove the clone here (Field is Copy)
There was a problem hiding this comment.
F is copy but Expr isn't (and we use JointCombiner rather than a field element there).
There was a problem hiding this comment.
ah I see. My tolerance for generics is usually pretty low ^^ I'm wondering how we can make this clearer. Maybe by changing F with F_or_Expr or something. Food for thought
There was a problem hiding this comment.
and then creating an actual trait F_or_Expr : Zero + One + ... and implement it on what we need
kimchi/src/circuits/gate.rs
Outdated
| pub fn combine_table_entry<'a, F: Field, I: DoubleEndedIterator<Item = &'a F>>( | ||
| pub fn combine_table_entry< | ||
| 'a, | ||
| F: 'a + Zero + One + Add + Mul + Clone, |
There was a problem hiding this comment.
I'm wondering why F: Field + Zero here is not enough?
Also I'm wondering how I'm supposed to read F: 'a? And why it doesn't work without that
There was a problem hiding this comment.
F may be a type that is defined within a function; if the scope of 'a is broader than the function, then the data outlives the type. Thus F: 'a.
| joint_combiner: K, | ||
| eval: &G, | ||
| ) -> K { | ||
| self.reduce(eval).evaluate(joint_combiner) |
There was a problem hiding this comment.
honestly I find this function really hard to parse.
- we have a
JointLookup<SingleLookup<F>>for someF: Zero + One + Add + Mul + Clone - we want a
Kfor someK: Zero + One + Add + Mul + Mul<F, Output = K> + Clone - in the mean time, we have some eval function that takes a
LocalPositionand return aK
it's just too generic to really grasp what is really going on IMO. Is there a way to make this clearer?
There was a problem hiding this comment.
I'm wondering if we can just have code duplication for the different instantiations of these functions, this would make the code much clearer imo and we wouldn't need to pass a closure around (IIUC)
| j.entry | ||
| .iter() | ||
| .enumerate() | ||
| .map(|(i, s)| E::constant(ConstantExpr::JointCombiner.pow(i as u64)) * single_lookup(s)) |
There was a problem hiding this comment.
so if I understand correctly this is replaced by the logic in combine_table_entry
This PR generalises
JointLookupso that it can handleSingleLookup<F>s,Fs, orExpr<ConstantExpr<F>>s as arguments, and relaxes the constraints oncombine_table_entry.This allows us to
JointLookupin the constraint system, rather than unpacking and re-packing itcombine_table_entryinto onecombine_table_entryminimises the number of multiplications compared to the others, so this is also an efficiency gaintable_ids