Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exported shared code to temp variables in ExprEvaluator. #893

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

Alon-Ti
Copy link
Contributor

@Alon-Ti Alon-Ti commented Nov 18, 2024

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@Alon-Ti Alon-Ti marked this pull request as ready for review November 18, 2024 16:40
Copy link
Contributor Author

@Alon-Ti Alon-Ti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

    #[test]
    fn test_expr_eval() {

This was a pain to keep up to date, doesn't mean much to the reader, and became kind of redundant with the repr test.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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: 0b14862 Previous: cd8b37b Ratio
iffts/simd ifft/22 12620057 ns/iter (± 260219) 6306399 ns/iter (± 210024) 2.00
merkle throughput/simd merkle 30039533 ns/iter (± 251957) 13712527 ns/iter (± 579195) 2.19

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

CC: @shaharsamocha7

@Alon-Ti Alon-Ti force-pushed the alont/formal-logup-sums-variables branch from 1ed3739 to 92a18c4 Compare November 19, 2024 12:55
@Alon-Ti Alon-Ti force-pushed the alont/formal-logup-sums-variables branch from 92a18c4 to 91ce7f8 Compare November 19, 2024 13:01
@Alon-Ti Alon-Ti force-pushed the alont/formal-logup-sums-variables branch from 91ce7f8 to c45d810 Compare November 19, 2024 13:06
@Alon-Ti Alon-Ti force-pushed the alont/formal-logup-sums branch 2 times, most recently from 57cde32 to 159714d Compare November 19, 2024 13:09
@Alon-Ti Alon-Ti force-pushed the alont/formal-logup-sums-variables branch from c45d810 to e88e970 Compare November 19, 2024 13:09
@codecov-commenter
Copy link

codecov-commenter commented Nov 19, 2024

Codecov Report

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

Project coverage is 91.92%. Comparing base (6de095b) to head (0b14862).
Report is 3 commits behind head on dev.

Files with missing lines Patch % Lines
crates/prover/src/constraint_framework/expr.rs 98.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #893      +/-   ##
==========================================
+ Coverage   91.83%   91.92%   +0.08%     
==========================================
  Files          93       93              
  Lines       13339    13721     +382     
  Branches    13339    13721     +382     
==========================================
+ Hits        12250    12613     +363     
- Misses        972      999      +27     
+ Partials      117      109       -8     

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

Copy link
Collaborator

@ohad-starkware ohad-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Previously, Alon-Ti wrote…

This was a pain to keep up to date, doesn't mean much to the reader, and became kind of redundant with the repr test.

yes this is not a good test

Copy link
Collaborator

@ohad-starkware ohad-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


crates/prover/src/constraint_framework/expr.rs line 245 at r3 (raw file):

    pub logup: FormalLogupAtRow,
    pub temp_vars: Vec<(String, Expr)>,
    cur_temp_var_index: usize,

.len()


crates/prover/src/constraint_framework/expr.rs line 255 at r3 (raw file):

            constraints: Default::default(),
            logup: FormalLogupAtRow::new(INTERACTION_TRACE_IDX, has_partial_sum, log_size),
            temp_vars: vec![],

Suggestion:

intermediates

Copy link
Collaborator

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


crates/prover/src/constraint_framework/expr.rs line 318 at r3 (raw file):

        } else {
            unreachable!();
        }

Change to match

Code quote:

        if let Expr::Mul(one, constraint) = Expr::one() * constraint {
            assert_eq!(*one, Expr::one());
            self.constraints.push(*constraint);
        } else {
            unreachable!();
        }

@Alon-Ti Alon-Ti force-pushed the alont/formal-logup-sums-variables branch from d88f160 to e350aa7 Compare November 24, 2024 16:18
Copy link
Contributor Author

@Alon-Ti Alon-Ti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)


crates/prover/src/constraint_framework/expr.rs line 245 at r3 (raw file):

Previously, ohad-starkware (Ohad) wrote…

.len()

Done.


crates/prover/src/constraint_framework/expr.rs line 318 at r3 (raw file):

Previously, shaharsamocha7 wrote…

Change to match

Done.


crates/prover/src/constraint_framework/expr.rs line 255 at r3 (raw file):

            constraints: Default::default(),
            logup: FormalLogupAtRow::new(INTERACTION_TRACE_IDX, has_partial_sum, log_size),
            temp_vars: vec![],

Done.

Copy link
Collaborator

@ohad-starkware ohad-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

    fn add_constraint<G>(&mut self, constraint: G)
    where
        Self::EF: std::ops::Mul<G, Output = Self::EF>,

add From to the trait function, and use .into here instead of all this

Code quote:

 Self::EF: std::ops::Mul<G, Output = Self::EF>

@Alon-Ti Alon-Ti changed the base branch from alont/formal-logup-sums to graphite-base/893 November 25, 2024 10:18
@Alon-Ti Alon-Ti force-pushed the alont/formal-logup-sums-variables branch from e350aa7 to 6501f2a Compare November 25, 2024 10:18
@Alon-Ti Alon-Ti changed the base branch from graphite-base/893 to dev November 25, 2024 10:18
@Alon-Ti Alon-Ti force-pushed the alont/formal-logup-sums-variables branch 2 times, most recently from 584af24 to 1a8cb5c Compare November 25, 2024 11:06
Copy link
Contributor Author

@Alon-Ti Alon-Ti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @ohad-starkware and @shaharsamocha7)


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

Previously, ohad-starkware (Ohad) wrote…

add From to the trait function, and use .into here instead of all this

Fixed in 901.

Copy link
Collaborator

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


crates/prover/src/constraint_framework/expr.rs line 265 at r5 (raw file):

    }

    pub fn add_temp_var(&mut self, expr: Expr) -> Expr {

do we want to call it temp_var or intermediate?
In all places

Code quote:

pub fn add_temp_var(&mut self, expr: Expr) -> Expr {

Copy link
Collaborator

@ohad-starkware ohad-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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

@Alon-Ti Alon-Ti force-pushed the alont/formal-logup-sums-variables branch from 1a8cb5c to 0b14862 Compare November 25, 2024 15:31
Copy link
Contributor Author

@Alon-Ti Alon-Ti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


crates/prover/src/constraint_framework/expr.rs line 265 at r5 (raw file):

Previously, shaharsamocha7 wrote…

do we want to call it temp_var or intermediate?
In all places

Done.

Copy link
Collaborator

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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

@Alon-Ti Alon-Ti merged commit 4d64300 into dev Nov 26, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants