Change Eval Framework Copy requirement to Clone.

[Alon-Ti]

This change is 

[Alon-Ti]

• Formal polynomial evaluator. #836
• Formal polynomials. #835
• Change Eval Framework Copy requirement to Clone. #834 👈
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Alon-Ti and the rest of your teammates on Graphite

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 97.64706% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.91%. Comparing base (ee8a9c6) to head (095fc4e).
Report is 1 commits behind head on dev.

Files with missing lines	Patch %	Lines
...es/prover/src/core/backend/simd/very_packed_m31.rs	0.00%	0 Missing and 1 partial ⚠️
crates/prover/src/core/lookups/utils.rs	95.65%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##              dev     #834   +/-   ##
=======================================
  Coverage   91.90%   91.91%           
=======================================
  Files          90       90           
  Lines       12364    12375   +11     
  Branches    12364    12375   +11     
=======================================
+ Hits        11363    11374   +11     
+ Misses        895      892    -3     
- Partials      106      109    +3     

Flag	Coverage Δ
	91.91% <97.64%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[andrewmilson]

Reviewable status: 0 of 23 files reviewed, 2 unresolved discussions (waiting on @Alon-Ti and @shaharsamocha7)

---

crates/prover/src/constraint_framework/mod.rs line 87 at r1 (raw file):

    ) -> [Self::EF; N] {
        let res_col_major = array::from_fn(|_| self.next_interaction_mask(interaction, offsets));
        array::from_fn(|i| Self::combine_ef(res_col_major.clone().map(|c| c[i].clone())))

Suggestion: without clone

Suggestion:

        let mut res_col_major =
            array::from_fn(|_| self.next_interaction_mask(interaction, offsets).into_iter());
        array::from_fn(|_| {
            Self::combine_ef(res_col_major.each_mut().map(|iter| iter.next().unwrap()))
        })

---

crates/prover/src/core/fields/mod.rs line 100 at r1 (raw file):

    + Neg<Output = Self>
    + ComplexConjugate
    + Copy

Does this need to have Copy removed?

Code quote:

+ Copy

[shaharsamocha7]

Reviewable status: 0 of 23 files reviewed, 3 unresolved discussions (waiting on @Alon-Ti and @andrewmilson)

a discussion (no related file):
Main change here is that we removed the Copy trait for Field?
Can you explain why is it better?

---

[Alon-Ti]

Reviewable status: 0 of 23 files reviewed, 3 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)

a discussion (no related file):

Previously, shaharsamocha7 wrote…

Main change here is that we removed the Copy trait for Field?
Can you explain why is it better?

We replace Copy with Clone, the reason is that Copy can only be implemented for objects that can be copied by copying the underlying binary repr (so they should generally be small and constant size), while the formal polynomials are not.

For types that have Copy, .clone() should use it and be "optimized out", so this shouldn't change what the current code compiles to.

---

---

crates/prover/src/constraint_framework/mod.rs line 87 at r1 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Suggestion: without clone

Done.

---

crates/prover/src/core/fields/mod.rs line 100 at r1 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Does this need to have Copy removed?

Done.

[andrewmilson]

Reviewable status: 0 of 23 files reviewed, 3 unresolved discussions (waiting on @shaharsamocha7)

[andrewmilson]

Reviewable status: 0 of 23 files reviewed, 4 unresolved discussions (waiting on @Alon-Ti and @shaharsamocha7)

---

crates/prover/src/constraint_framework/logup.rs line 51 at r3 (raw file):

        // Add a constraint that num / denom = diff.
        if let Some(cur_frac) = self.cur_frac.clone() {
            let cur_cumsum = eval.next_extension_interaction_mask(self.interaction, [0])[0].clone();

Suggestion:

let [cur_cumsum] = eval.next_extension_interaction_mask(self.interaction, [0]);

[andrewmilson]

with comments

Reviewable status: 0 of 23 files reviewed, 7 unresolved discussions (waiting on @Alon-Ti and @shaharsamocha7)

---

crates/prover/src/examples/blake/round/constraints.rs line 203 at r3 (raw file):

            numerator: comb0.clone() + comb1.clone(),
            denominator: comb0 * comb1,
        };

Suggestion:

Reciprocal::new(comb0) + Reciprocal::new(comb1)

---

crates/prover/src/examples/blake/scheduler/constraints.rs line 40 at r3 (raw file):

        logup.write_frac(
            eval,
            Fraction::new(denom_i.clone() + denom_j.clone(), denom_i * denom_j),

Suggestion:

Reciprocal::new(denom_i) + Reciprocal::new(denom_j)

---

crates/prover/src/examples/poseidon/mod.rs line 198 at r3 (raw file):

                final_state_denom.clone() - initial_state_denom.clone(),
                initial_state_denom.clone() * final_state_denom.clone(),
            ),

Suggestion:

            Reciprocal::new(final_state_denom) - Reciprocal::new(initial_state_denom),

[Alon-Ti]

Reviewable status: 0 of 23 files reviewed, 7 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)

---

crates/prover/src/constraint_framework/logup.rs line 51 at r3 (raw file):

        // Add a constraint that num / denom = diff.
        if let Some(cur_frac) = self.cur_frac.clone() {
            let cur_cumsum = eval.next_extension_interaction_mask(self.interaction, [0])[0].clone();

Done.

---

crates/prover/src/examples/blake/round/constraints.rs line 203 at r3 (raw file):

            numerator: comb0.clone() + comb1.clone(),
            denominator: comb0 * comb1,
        };

Nice!

---

crates/prover/src/examples/blake/scheduler/constraints.rs line 40 at r3 (raw file):

        logup.write_frac(
            eval,
            Fraction::new(denom_i.clone() + denom_j.clone(), denom_i * denom_j),

Done.

---

crates/prover/src/examples/poseidon/mod.rs line 198 at r3 (raw file):

                final_state_denom.clone() - initial_state_denom.clone(),
                initial_state_denom.clone() * final_state_denom.clone(),
            ),

Done.

[andrewmilson]

Reviewable status: 0 of 23 files reviewed, 7 unresolved discussions (waiting on @shaharsamocha7)

[Alon-Ti]

Merge activity

• Sep 25, 9:20 AM EDT: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..

[andrewmilson]

Reviewable status: 0 of 23 files reviewed, 6 unresolved discussions (waiting on @shaharsamocha7)

.