Expr formatting.

[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 👈 (View in Graphite)
• Formal Expr logup variables. #887
• 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 1 files reviewed, 3 unresolved discussions (waiting on @Alon-Ti)

---

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

                d.format_expr()
            ),
            Expr::Const(c) => c.0.to_string(),

Const is a BaseField so why are you treat it as a string?

Code quote:

Expr::Const(c) => c.0.to_string(),

---

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

            Expr::Var(v) => v.to_string(),
            Expr::Add(a, b) => format!("{} + {}", a.format_expr(), b.format_expr()),
            Expr::Sub(a, b) => format!("{} - ({})", a.format_expr(), b.format_expr()),

don't you need to add parenthesis on this too?

Suggestion:

Expr::Sub(a, b) => format!("({}) - ({})", a.format_expr(), b.format_expr())

---

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

        let eval = test_struct.evaluate(ExprEvaluator::new(16, (SecureField::zero(), None)));
        assert_eq!(eval.constraints[0].format_expr(), "(1) * ((((col_1_0[0]) * (col_1_1[0])) * (col_1_2[0])) * (1/(col_1_0[0] + col_1_1[0])))");
        assert_eq!(eval.constraints[1].format_expr(), "(1) * ((SecureCol(col_2_4[0], col_2_6[0], col_2_8[0], col_2_10[0]) - (SecureCol(col_2_5[18446744073709551615], col_2_7[18446744073709551615], col_2_9[18446744073709551615], col_2_11[18446744073709551615]) - ((col_0_3[0]) * (SecureCol(0, 0, 0, 0)))) - (0)) * (0 + (TestRelation_alpha0) * (col_1_0[0]) + (TestRelation_alpha1) * (col_1_1[0]) + (TestRelation_alpha2) * (col_1_2[0]) - (TestRelation_z)) - (1))");

I think you should write it in a clearer indentation, (so it will be easy to understand what the constraints are, and then you it in the test.

Code quote:

        assert_eq!(eval.constraints[0].format_expr(), "(1) * ((((col_1_0[0]) * (col_1_1[0])) * (col_1_2[0])) * (1/(col_1_0[0] + col_1_1[0])))");
        assert_eq!(eval.constraints[1].format_expr(), "(1) * ((SecureCol(col_2_4[0], col_2_6[0], col_2_8[0], col_2_10[0]) - (SecureCol(col_2_5[18446744073709551615], col_2_7[18446744073709551615], col_2_9[18446744073709551615], col_2_11[18446744073709551615]) - ((col_0_3[0]) * (SecureCol(0, 0, 0, 0)))) - (0)) * (0 + (TestRelation_alpha0) * (col_1_0[0]) + (TestRelation_alpha1) * (col_1_1[0]) + (TestRelation_alpha2) * (col_1_2[0]) - (TestRelation_z)) - (1))");

[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: 1caa5eb	Previous: f6214d1	Ratio
merkle throughput/simd merkle	29815729 ns/iter (± 440489)	14690867 ns/iter (± 434150)	2.03

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

CC: @shaharsamocha7

[Alon-Ti]

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

---

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

Previously, shaharsamocha7 wrote…

Const is a BaseField so why are you treat it as a string?

Oh, right, the unreduced form :(

---

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

Previously, shaharsamocha7 wrote…

don't you need to add parenthesis on this too?

Isn't it correct like this? I think add doesn't need parens, sub needs parens on the negative part and mul needs parens on both.

a * b - d + c - (x * y + z) <=> (a * b - d + c) - (x * y + z)

---

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

Previously, shaharsamocha7 wrote…

I think you should write it in a clearer indentation, (so it will be easy to understand what the constraints are, and then you it in the test.

Done.

[shaharsamocha7]

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

[codecov-commenter]

Codecov Report

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

Project coverage is 91.87%. Comparing base (96f7025) to head (1caa5eb).
Report is 1 commits behind head on dev.

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

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #888      +/-   ##
==========================================
+ Coverage   91.85%   91.87%   +0.01%     
==========================================
  Files          93       93              
  Lines       13053    13097      +44     
  Branches    13053    13097      +44     
==========================================
+ Hits        11990    12033      +43     
- Misses        948      949       +1     
  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