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

Added total_ and claimed_sums as formal variables in ExprEvaluator. #892

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

Alon-Ti
Copy link
Contributor

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

No description provided.

@Alon-Ti Alon-Ti marked this pull request as ready for review November 18, 2024 12:08
@reviewable-StarkWare
Copy link

This change is Reviewable

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: e49c0a4 Previous: cd8b37b Ratio
iffts/simd ifft/22 12947109 ns/iter (± 265070) 6306399 ns/iter (± 210024) 2.05
merkle throughput/simd merkle 30514622 ns/iter (± 269483) 13712527 ns/iter (± 579195) 2.23

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

CC: @shaharsamocha7

@Alon-Ti Alon-Ti force-pushed the alont/state-machine-expr branch from 5884cb3 to d998220 Compare November 19, 2024 11:11
@Alon-Ti Alon-Ti force-pushed the alont/formal-logup-sums branch from 0036fae to 8750fad Compare November 19, 2024 11:11
@Alon-Ti Alon-Ti mentioned this pull request Nov 19, 2024
@Alon-Ti Alon-Ti force-pushed the alont/formal-logup-sums branch from 8750fad to 229f7ab Compare November 19, 2024 11:51
@Alon-Ti Alon-Ti force-pushed the alont/state-machine-expr branch 2 times, most recently from 1f7512f to ab4bb77 Compare November 19, 2024 11:57
@Alon-Ti Alon-Ti force-pushed the alont/formal-logup-sums branch from 229f7ab to 3fab82f Compare November 19, 2024 11:57
@Alon-Ti Alon-Ti force-pushed the alont/state-machine-expr branch from ab4bb77 to a0c3a0d Compare November 19, 2024 12:55
@Alon-Ti Alon-Ti force-pushed the alont/formal-logup-sums branch 3 times, most recently from 794145d to 7a94c0a Compare November 19, 2024 13:06
@Alon-Ti Alon-Ti changed the base branch from alont/state-machine-expr to graphite-base/892 November 19, 2024 13:08
@Alon-Ti Alon-Ti force-pushed the alont/formal-logup-sums branch from 7a94c0a to 57cde32 Compare November 19, 2024 13:08
@Alon-Ti Alon-Ti changed the base branch from graphite-base/892 to dev November 19, 2024 13:09
@Alon-Ti Alon-Ti force-pushed the alont/formal-logup-sums branch from 57cde32 to 159714d Compare November 19, 2024 13:09
@codecov-commenter
Copy link

codecov-commenter commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.83%. Comparing base (77de4d7) to head (159714d).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #892      +/-   ##
==========================================
- Coverage   91.95%   91.83%   -0.13%     
==========================================
  Files          93       93              
  Lines       13097    13111      +14     
  Branches    13097    13111      +14     
==========================================
- Hits        12043    12040       -3     
- Misses        939      956      +17     
  Partials      115      115              

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


🚨 Try these New Features:

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 3 files reviewed, 3 unresolved discussions (waiting on @Alon-Ti and @shaharsamocha7)


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

// (1 << 31) - 1 is an offset no column can reach, it signifies the variable
// offset, which is an input to the verifier.
const CLAIMED_SUM_DUMMY_OFFSET: usize = (1 << 31) - 1;

you can take m31::P

Code quote:

onst CLAIMED_SUM_DUMMY_OFFSET: usize = (1 << 31) - 1;

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

    pub fn new(interaction: usize, has_partial_sum: bool, log_size: u32) -> Self {
        let total_sum_name = "total_sum".to_string();
        let claimed_sum_name = "claimed_sum".to_string();

shouldnt they be specific per component?

Code quote:

        let total_sum_name = "total_sum".to_string();
        let claimed_sum_name = "claimed_sum".to_string();

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

            total_sum: Expr::Param(total_sum_name),
            claimed_sum: has_partial_sum
                .then_some((Expr::Param(claimed_sum_name), CLAIMED_SUM_DUMMY_OFFSET)),

if you dont care about the offest why is it here

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 3 files reviewed, 3 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)


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

Previously, ohad-starkware (Ohad) wrote…

you can take m31::P

Done.


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

Previously, ohad-starkware (Ohad) wrote…

shouldnt they be specific per component?

There's no access to the component name here (nor really a notion of "component"), the way I think it would work is that every component will have a constraint evaluator function that will take "*_sum" as parameters, and the external glue code, that knows about components, will send them accordingly.

It's also possible that we may want to have some sort of "scoping" mechanism, but I'm saving that for when there are named columns because we can work around it here.


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

Previously, ohad-starkware (Ohad) wrote…

if you dont care about the offest why is it here

To reuse the rest of the logup code, there has to be some offset, this sends the dummy offset that the code on line 47 interprets as a param.

@Alon-Ti Alon-Ti force-pushed the alont/formal-logup-sums branch from 159714d to e49c0a4 Compare November 20, 2024 11:49
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 3 files at r1, 1 of 1 files at r4, all commit messages.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @ohad-starkware)

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:

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

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