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

PolyOps interpolate columns refactor #823

Merged

Conversation

jarnesino
Copy link
Contributor

@jarnesino jarnesino commented Sep 5, 2024

This PR introduces a new PolyOps trait function allows any backend to optimize interpolations of many columns.


This change is Reviewable

@jarnesino jarnesino marked this pull request as ready for review September 5, 2024 15:29
@jarnesino jarnesino changed the title Optimize interpolation for wide traces PolyOps interpolate columns refactor Sep 5, 2024
@jarnesino jarnesino force-pushed the optimize-interpolation branch 4 times, most recently from 4b06605 to 5398d91 Compare September 9, 2024 17:21
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, 3 unresolved discussions (waiting on @jarnesino)


crates/prover/src/core/pcs/prover.rs line 174 at r1 (raw file):

        let span = span!(Level::INFO, "Interpolation for commitment").entered();
        let polys: Vec<CirclePoly<B>> =
            B::interpolate_columns(columns, self.commitment_scheme.twiddles);

Suggestion:

        let polys = B::interpolate_columns(columns, self.commitment_scheme.twiddles);

crates/prover/src/core/poly/circle/ops.rs line 31 at r1 (raw file):

    /// Computes minimal polynomials that evaluate to the same values as each evaluation in `columns`.
    /// Used by the [`TreeBuilder::extend_evals()`] function.

discussed with team,
this doc is redundant; 'interpolate' is clear enough.

Code quote:

    /// Computes minimal polynomials that evaluate to the same values as each evaluation in `columns`.
    /// Used by the [`TreeBuilder::extend_evals()`] function.

crates/prover/src/core/poly/circle/ops.rs line 38 at r1 (raw file):

        columns
            .into_iter()
            .map(|eval| eval.interpolate_with_twiddles(&twiddles))

clippy hates the ref
try to run scripts/clippy.sh

Suggestion:

.map(|eval| eval.interpolate_with_twiddles(twiddles))

@jarnesino
Copy link
Contributor Author

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @jarnesino)

crates/prover/src/core/pcs/prover.rs line 174 at r1 (raw file):

        let span = span!(Level::INFO, "Interpolation for commitment").entered();
        let polys: Vec<CirclePoly<B>> =
            B::interpolate_columns(columns, self.commitment_scheme.twiddles);

Suggestion:

        let polys = B::interpolate_columns(columns, self.commitment_scheme.twiddles);

crates/prover/src/core/poly/circle/ops.rs line 31 at r1 (raw file):

    /// Computes minimal polynomials that evaluate to the same values as each evaluation in `columns`.
    /// Used by the [`TreeBuilder::extend_evals()`] function.

discussed with team, this doc is redundant; 'interpolate' is clear enough.

Code quote:

    /// Computes minimal polynomials that evaluate to the same values as each evaluation in `columns`.
    /// Used by the [`TreeBuilder::extend_evals()`] function.

crates/prover/src/core/poly/circle/ops.rs line 38 at r1 (raw file):

        columns
            .into_iter()
            .map(|eval| eval.interpolate_with_twiddles(&twiddles))

clippy hates the ref try to run scripts/clippy.sh

Suggestion:

.map(|eval| eval.interpolate_with_twiddles(twiddles))

Changes made.
Thanks for the comments!

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 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jarnesino and @ohad-starkware)

@shaharsamocha7 shaharsamocha7 merged commit bb0cce2 into starkware-libs:dev Oct 7, 2024
13 of 15 checks passed
@jarnesino jarnesino deleted the optimize-interpolation branch October 7, 2024 15:20
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