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

Eval framework info #715

Merged
merged 1 commit into from
Jul 14, 2024
Merged

Eval framework info #715

merged 1 commit into from
Jul 14, 2024

Conversation

spapinistarkware
Copy link
Contributor

@spapinistarkware spapinistarkware commented Jul 9, 2024

This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2024

Codecov Report

Attention: Patch coverage is 0% with 26 lines in your changes missing coverage. Please review.

Project coverage is 90.20%. Comparing base (a69e8ca) to head (890386f).

Files Patch % Lines
crates/prover/src/constraint_framework/info.rs 0.00% 26 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #715      +/-   ##
==========================================
- Coverage   90.43%   90.20%   -0.23%     
==========================================
  Files          80       81       +1     
  Lines       10328    10354      +26     
  Branches    10328    10354      +26     
==========================================
  Hits         9340     9340              
- Misses        904      930      +26     
  Partials       84       84              

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

@spapinistarkware spapinistarkware force-pushed the eval_framework_asserts branch from 05452dd to 86be1d9 Compare July 10, 2024 06:39
@spapinistarkware spapinistarkware force-pushed the eval_framework_asserts branch 3 times, most recently from d1f2d95 to d4bdc02 Compare July 14, 2024 08:48
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 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @spapinistarkware)


crates/prover/src/constraint_framework/info.rs line 11 at r1 (raw file):

#[derive(Default)]
pub struct InfoEvaluator {

Document that this is used to collect the mask offsets and count the constraints for a specific component.

Code quote:

pub struct InfoEvaluator {

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

        if self.mask_offsets.len() <= interaction {
            self.mask_offsets.resize(interaction + 1, vec![]);
        }

This code assumes that 'interaction` is continuous (which is true) but it may be called differently
Alternatively handle that differently

Suggestion:

        if self.mask_offsets.len() <= interaction {
            assert_eq(self.mask_offsets.len() == interaction + 1);
            self.mask_offsets.resize(interaction + 1, vec![]);
        }

crates/prover/src/constraint_framework/info.rs line 41 at r1 (raw file):

    }

    fn combine_ef(_values: [Self::F; 4]) -> Self::EF {

Fix

Suggestion:

    fn combine_ef(values: [Self::F; SECURE_EXTENSION_DEGREE]) -> Self::EF {

@spapinistarkware spapinistarkware force-pushed the eval_framework_asserts branch from d4bdc02 to 7074467 Compare July 14, 2024 11:47
@spapinistarkware spapinistarkware force-pushed the eval_framework_info branch 2 times, most recently from 0d809f0 to 3f50cef Compare July 14, 2024 11:54
Copy link
Contributor Author

@spapinistarkware spapinistarkware 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, 3 unresolved discussions (waiting on @shaharsamocha7)


crates/prover/src/constraint_framework/info.rs line 11 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Document that this is used to collect the mask offsets and count the constraints for a specific component.

Done.


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

Previously, shaharsamocha7 wrote…

This code assumes that 'interaction` is continuous (which is true) but it may be called differently
Alternatively handle that differently

As discussed.


crates/prover/src/constraint_framework/info.rs line 41 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Fix

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 1 of 2 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @spapinistarkware)

@spapinistarkware spapinistarkware force-pushed the eval_framework_asserts branch 2 times, most recently from 03ac5c2 to ef45c3c Compare July 14, 2024 12:06
Copy link
Contributor Author

spapinistarkware commented Jul 14, 2024

Merge activity

@spapinistarkware spapinistarkware changed the base branch from eval_framework_asserts to dev July 14, 2024 12:18
@spapinistarkware spapinistarkware merged commit 2c6cbfd into dev Jul 14, 2024
14 checks passed
@spapinistarkware spapinistarkware mentioned this pull request Aug 7, 2024
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.

3 participants