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

Optimized lookup combine #765

Merged
merged 1 commit into from
Aug 5, 2024

Conversation

spapinistarkware
Copy link
Contributor

@spapinistarkware spapinistarkware commented Jul 28, 2024

This change is Reviewable

@spapinistarkware spapinistarkware mentioned this pull request Jul 28, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2024

Codecov Report

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

Project coverage is 93.34%. Comparing base (17591ea) to head (4351076).

Files Patch % Lines
crates/prover/src/constraint_framework/logup.rs 96.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #765      +/-   ##
==========================================
+ Coverage   93.27%   93.34%   +0.07%     
==========================================
  Files          89       89              
  Lines       11948    11957       +9     
  Branches    11948    11957       +9     
==========================================
+ Hits        11144    11161      +17     
+ Misses        703      695       -8     
  Partials      101      101              

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

@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from 7fad3c5 to 5af79fa Compare July 29, 2024 14:05
@spapinistarkware spapinistarkware mentioned this pull request Jul 29, 2024
@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from 5af79fa to db98a63 Compare July 30, 2024 11:11
@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from db98a63 to c28ed04 Compare July 30, 2024 12:56
@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from c28ed04 to 786d728 Compare July 30, 2024 12:59
@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from 786d728 to 17e294d Compare July 30, 2024 13:12
@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from 17e294d to 87a136c Compare July 30, 2024 13:32
@spapinistarkware spapinistarkware mentioned this pull request Jul 30, 2024
@spapinistarkware spapinistarkware force-pushed the spapini/07-28-blake_xor_table branch 2 times, most recently from cb2b625 to 73ba6c4 Compare August 1, 2024 07:08
@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from 87a136c to a27e12e Compare August 1, 2024 07:08
@spapinistarkware spapinistarkware mentioned this pull request Aug 1, 2024
@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from a27e12e to d3fe674 Compare August 5, 2024 06:17
@spapinistarkware spapinistarkware changed the base branch from spapini/07-28-blake_xor_table to dev August 5, 2024 06:17
@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from d3fe674 to fe41cc2 Compare August 5, 2024 07:09
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 2 of 9 files at r2, 7 of 7 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @spapinistarkware)


crates/prover/src/constraint_framework/logup.rs line 118 at r4 (raw file):

    pub alpha: SecureField,
    /// Precomputed powers of alpha.
    powers: Vec<SecureField>,

Rename

Suggestion:

alpha_powers: Vec<SecureField>,

crates/prover/src/constraint_framework/logup.rs line 121 at r4 (raw file):

}
impl LookupElements {
    pub fn draw(channel: &mut Blake2sChannel, n_powers: usize) -> Self {

Rename

Suggestion:

n_alpha_powers: usize

crates/prover/src/examples/poseidon/mod.rs line 371 at r4 (raw file):

    span.exit();

    // Draw lookup element.

non-blocking

Suggestion:

   // Draw lookup elements.

@spapinistarkware spapinistarkware force-pushed the spapini/07-28-optimized_lookup_combine branch from fe41cc2 to 4351076 Compare August 5, 2024 08:45
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: 8 of 10 files reviewed, 2 unresolved discussions (waiting on @shaharsamocha7)


crates/prover/src/constraint_framework/logup.rs line 118 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Rename

Done.


crates/prover/src/constraint_framework/logup.rs line 121 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Rename

This goes away next PR, and I'd ike to avoid conflicts:)

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 r5.
Reviewable status: 9 of 10 files reviewed, all discussions resolved (waiting on @spapinistarkware)

@spapinistarkware spapinistarkware merged commit 2baab80 into dev Aug 5, 2024
13 of 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