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

Remove claimed_sum and LogupSums #979

Open
wants to merge 1 commit into
base: 01-12-Logup_cumsum_constraint_with_cumsum_shift
Choose a base branch
from

Conversation

shaharsamocha7
Copy link
Collaborator

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator Author

shaharsamocha7 commented Jan 12, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

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

@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.27%. Comparing base (3ca832e) to head (91ce246).

Additional details and impacted files
@@                                 Coverage Diff                                 @@
##           01-12-Logup_cumsum_constraint_with_cumsum_shift     #979      +/-   ##
===================================================================================
+ Coverage                                            92.02%   92.27%   +0.25%     
===================================================================================
  Files                                                  105      105              
  Lines                                                14274    14221      -53     
  Branches                                             14274    14221      -53     
===================================================================================
- Hits                                                 13135    13123      -12     
+ Misses                                                1064     1024      -40     
+ Partials                                                75       74       -1     

☔ 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.

:lgtm:

Reviewed 21 of 21 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @shaharsamocha7)

@shaharsamocha7 shaharsamocha7 force-pushed the 01-12-Logup_cumsum_constraint_with_cumsum_shift branch from ec20bc8 to bbbe8a7 Compare January 13, 2025 08:18
@shaharsamocha7 shaharsamocha7 force-pushed the 01-12-Remove_claimed_sum_and_LogupSums branch 2 times, most recently from 9eb9411 to d65de78 Compare January 13, 2025 08:25
@shaharsamocha7 shaharsamocha7 force-pushed the 01-12-Logup_cumsum_constraint_with_cumsum_shift branch from bbbe8a7 to 9e789d3 Compare January 14, 2025 12:44
@shaharsamocha7 shaharsamocha7 force-pushed the 01-12-Remove_claimed_sum_and_LogupSums branch 2 times, most recently from b1537e1 to 7b0dc24 Compare January 14, 2025 14:01
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 1 of 1 files at r2, 11 of 11 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @shaharsamocha7)


crates/prover/src/constraint_framework/info.rs line 30 at r3 (raw file):

}
impl InfoEvaluator {
    pub fn new(log_size: u32, preprocessed_columns: Vec<String>, total_sum: SecureField) -> Self {

isnt this a Gali change?

Copy link
Collaborator Author

@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.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @shaharsamocha7)


crates/prover/src/constraint_framework/info.rs line 30 at r3 (raw file):

Previously, ohad-starkware (Ohad) wrote…

isnt this a Gali change?

I rebased over her change.
If you check r0->r3 the change is only on total_sum

@shaharsamocha7 shaharsamocha7 force-pushed the 01-12-Logup_cumsum_constraint_with_cumsum_shift branch from 9e789d3 to 3ca832e Compare January 14, 2025 17:28
@shaharsamocha7 shaharsamocha7 force-pushed the 01-12-Remove_claimed_sum_and_LogupSums branch from 7b0dc24 to 91ce246 Compare January 14, 2025 17:28
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.

4 participants