Formal Expr logup variables.

[Alon-Ti]

No description provided.

[reviewable-StarkWare]

This change is 

[Alon-Ti]

• Added add_intermediate and add_secure_intermediate to eval API. #919
• Add safe simplify for expressions that compares random assignments before and after. #918
• Added test for expression simplifier. #908
• Added eval on exprs. #904
• Separated Base and Extension field expressions. #903
• Added From<G> in add_constraint. #901
• Simplify expressions. #894
• Exported shared code to temp variables in ExprEvaluator. #893
• Added total_ and claimed_sums as formal variables in ExprEvaluator. #892
• State machine constraint string. #889
• Expr formatting. #888
• Formal Expr logup variables. #887 👈 (View in Graphite)
• Initialize logups automatically when first used. #879 : 1 other dependent PR (#883 )
• Abstracted eval.write_fracs to eval.add_to_relations. #876
• Added LogupAtRow functionality to EvalAtRow. #875
• dev

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

[shaharsamocha7]

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @Alon-Ti)

---

crates/prover/src/constraint_framework/expr.rs line 30 at r1 (raw file):

    SecureCol([Box<Expr>; 4]),
    Const(BaseField),
    Var(String),

consider to rename

Suggestion:

ExternalVar(String),

---

crates/prover/src/constraint_framework/expr.rs line 30 at r1 (raw file):

    SecureCol([Box<Expr>; 4]),
    Const(BaseField),
    Var(String),

Should it also help with preprocessed columns?
Document

Code quote:

Var(String),

---

crates/prover/src/constraint_framework/expr.rs line 155 at r1 (raw file):

    let z = Expr::Var(relation.get_name().to_owned() + "_z");
    let alpha_powers = (0..relation.get_size())
        .map(|i| Expr::Var(relation.get_name().to_owned() + "_alpha" + &i.to_string()));

Can you put those strings as consts in the top of the function?

Suggestion:

"_alpha", "z"

---

crates/prover/src/constraint_framework/expr.rs line 232 at r1 (raw file):

                     values,
                 }| {
                    Fraction::new(multiplicity.clone(), combine_formal(*relation, values))

Can't it use the default impl for add_to_relation?
this is because the combine_formal?

Code quote:

Fraction::new(multiplicity.clone(), combine_formal(*relation, values))

---

crates/prover/src/constraint_framework/expr.rs line 333 at r1 (raw file):

                                            interaction: 2,
                                            idx: 5,
                                            offset: 18446744073709551615

Why we got this nubmer? (1 << 64)

Code quote:

offset: 18446744073709551615

[Alon-Ti]

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

---

crates/prover/src/constraint_framework/expr.rs line 30 at r1 (raw file):

Previously, shaharsamocha7 wrote…

consider to rename

Param?

---

crates/prover/src/constraint_framework/expr.rs line 30 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Should it also help with preprocessed columns?
Document

Preprocessed columns are just columns that exist in the trace, I think the expr evaluator doesn't care how they're generated and who has access to them, at least until we have named columns in general.

---

crates/prover/src/constraint_framework/expr.rs line 155 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Can you put those strings as consts in the top of the function?

Done.

---

crates/prover/src/constraint_framework/expr.rs line 232 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Can't it use the default impl for add_to_relation?
this is because the combine_formal?

Yes. combine is a property of the relation itself, so it can't vary by evaluator.

---

crates/prover/src/constraint_framework/expr.rs line 333 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Why we got this nubmer? (1 << 64)

Done.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 99.36709% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.85%. Comparing base (3a88b3a) to head (9037ea4).
Report is 1 commits behind head on dev.

Files with missing lines	Patch %	Lines
crates/prover/src/constraint_framework/expr.rs	99.35%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #887      +/-   ##
==========================================
+ Coverage   91.61%   91.85%   +0.24%     
==========================================
  Files          93       93              
  Lines       12901    13053     +152     
  Branches    12901    13053     +152     
==========================================
+ Hits        11819    11990     +171     
+ Misses        967      948      -19     
  Partials      115      115              

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

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite	Current: 9037ea4	Previous: f6214d1	Ratio
merkle throughput/simd merkle	32211634 ns/iter (± 488417)	14690867 ns/iter (± 434150)	2.19

This comment was automatically generated by workflow using github-action-benchmark.

CC: @shaharsamocha7

[shaharsamocha7]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Alon-Ti)

---

crates/prover/src/constraint_framework/expr.rs line 30 at r1 (raw file):

Previously, Alon-Ti wrote…

Preprocessed columns are just columns that exist in the trace, I think the expr evaluator doesn't care how they're generated and who has access to them, at least until we have named columns in general.

so it should access them by index same as any other column?

---

crates/prover/src/constraint_framework/expr.rs line 30 at r2 (raw file):

    SecureCol([Box<Expr>; 4]),
    Const(BaseField),
    /// A parameter external to the AIR, for example the interaction elements of a relation.

why is it called external?

Suggestion:

/// External parameter to the AIR, for example the interaction elements of a relation.

[shaharsamocha7]

Reviewed all commit messages.
Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @Alon-Ti)

[shaharsamocha7]

Reviewed 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Alon-Ti)

[Alon-Ti]

Merge activity

• Nov 18, 11:45 AM EST: A user merged this pull request with Graphite.