Skip to content

Conversation

Gali-StarkWare
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

Gali-StarkWare commented Apr 29, 2025

Copy link
Contributor

@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 2 files at r1, all commit messages.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @Alon-Ti)


crates/prover/src/core/constraints.rs line 37 at r1 (raw file):

}

pub fn coset_vanishing_derivative<F: ExtensionOf<BaseField>>(coset: Coset, p: CirclePoint<F>) -> F {

Which points are zeros for this function?
Why do we have exp = 4**(log_size - 1)?
document

Code quote:

pub fn coset_vanishing_derivative<F: ExtensionOf<BaseField>>(coset: Coset, p: CirclePoint<F>) -> F {

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

// TODO(Gali): Remove.
#[allow(dead_code)]
fn weights(log_size: u32, sample_point: CirclePoint<SecureField>) -> Col<CpuBackend, SecureField> {

Is this going to be removed?
Or do we want to keep trivial weight calculation?

Code quote:

#[allow(dead_code)]
fn weights(log_size: u32, sample_point: CirclePoint<SecureField>) -> Col<CpuBackend, SecureField> {

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

                .to_vec(),
        );
        let s = CanonicCoset::new(3);

Can we evaluate on a larger CanonicCoset here?

Code quote:

        let s = CanonicCoset::new(3);

Copy link
Contributor

@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: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @Alon-Ti and @Gali-StarkWare)


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

Previously, shaharsamocha7 wrote…

Is this going to be removed?
Or do we want to keep trivial weight calculation?

also we would probably need the talk to understand this code

@Gali-StarkWare Gali-StarkWare force-pushed the gali/cpu_barycentric branch from 92a5086 to 8abe2fe Compare May 14, 2025 07:25
@codecov-commenter
Copy link

codecov-commenter commented May 14, 2025

Codecov Report

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

Project coverage is 92.68%. Comparing base (61d1f6b) to head (34fce22).
Report is 1 commits behind head on gali/barycentric.

Files with missing lines Patch % Lines
crates/prover/src/core/poly/circle/evaluation.rs 98.83% 1 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                  @@
##           gali/barycentric    #1068      +/-   ##
====================================================
+ Coverage             92.64%   92.68%   +0.04%     
====================================================
  Files                   106      106              
  Lines                 14273    14374     +101     
  Branches              14273    14374     +101     
====================================================
+ Hits                  13223    13323     +100     
- Misses                  969      970       +1     
  Partials                 81       81              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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: 34fce22 Previous: dbfd84c Ratio
iffts/simd ifft/26 267972097 ns/iter (± 2556591) 127397717 ns/iter (± 2072975) 2.10
iffts/simd ifft/27 621739418 ns/iter (± 8616375) 259960823 ns/iter (± 9389998) 2.39
iffts/simd ifft/28 1251160891 ns/iter (± 12913172) 562251429 ns/iter (± 4959325) 2.23
merkle throughput/simd merkle 29257607 ns/iter (± 548243) 13349252 ns/iter (± 275840) 2.19

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

CC: @shaharsamocha7

Copy link
Contributor Author

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


crates/prover/src/core/constraints.rs line 37 at r1 (raw file):

Previously, shaharsamocha7 wrote…

Which points are zeros for this function?
Why do we have exp = 4**(log_size - 1)?
document

Done.


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

Previously, shaharsamocha7 wrote…

also we would probably need the talk to understand this code

I am in favor of keeping the cpu weight calculation, like you have the cpu version of eval_at_point


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

Previously, shaharsamocha7 wrote…

Can we evaluate on a larger CanonicCoset here?

Will do

Copy link
Contributor

@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 all commit messages.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @Alon-Ti and @Gali-StarkWare)

Copy link
Contributor

@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: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @Alon-Ti and @Gali-StarkWare)


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

Previously, Gali-StarkWare wrote…

I am in favor of keeping the cpu weight calculation, like you have the cpu version of eval_at_point

so you we have a TODO to remove it?


crates/prover/src/core/constraints.rs line 37 at r3 (raw file):

}

/// Evaluates the formal derivative of a vanishing polynomial of the coset at a point.

Consider to change

Suggestion:

/// Evaluates the formal derivative of a vanishing polynomial of the coset at a point `p`.

crates/prover/src/core/constraints.rs line 42 at r3 (raw file):

    // of all cosets of smallet size than the given coset at the given point by 4^(log_size-1),
    // where log_size is the log size of the coset. This is because of the chain rule (also see
    // remark 15 in circle stark article).

you can also add a link to the circle stark paper

Suggestion:

    // The formal derivative is computed by multiplying the product of the vanishing polynomials of
    // of all cosets of smallet size than the given coset at the given point by 4^(log_size-1).
    // See Remark 15 in circle stark paper).

crates/prover/src/core/constraints.rs line 44 at r3 (raw file):

    // remark 15 in circle stark article).

    let field_four = F::one() + F::one() + F::one() + F::one();

NIT use from_u32_unchecked

Code quote:

F::one() + F::one() + F::one() + F::one()

crates/prover/src/core/poly/circle/evaluation.rs line 168 at r3 (raw file):

/// point p and domain size. The domain is the circle domain corresponding to the canonic coset of
/// the size provided.
fn weights(log_size: u32, sample_point: CirclePoint<SecureField>) -> Col<CpuBackend, SecureField> {

Why not get the coset directly?
Also rename function and change doc accordingly

Suggestion:

/// Computes the weights for Barycentric Lagrange interpolation for point `p` on `coset`.
fn barycentric_weights(coset: CanonicCoset, sample_point: CirclePoint<SecureField>) -> Col<CpuBackend, SecureField> {

Copy link
Contributor

@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: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @Alon-Ti and @Gali-StarkWare)


crates/prover/src/core/poly/circle/evaluation.rs line 168 at r3 (raw file):

/// point p and domain size. The domain is the circle domain corresponding to the canonic coset of
/// the size provided.
fn weights(log_size: u32, sample_point: CirclePoint<SecureField>) -> Col<CpuBackend, SecureField> {

Can we write here the simplest way to evaluate the barycentric_weights?

Code quote:

fn weights(log_size: u32, sample_point: CirclePoint<SecureField>) -> Col<CpuBackend, SecureField> {

Copy link
Contributor Author

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


crates/prover/src/core/constraints.rs line 42 at r3 (raw file):

Previously, shaharsamocha7 wrote…

you can also add a link to the circle stark paper

Done.


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

Previously, shaharsamocha7 wrote…

so you we have a TODO to remove it?

It's for the allow dead code


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

Previously, Gali-StarkWare wrote…

Will do

Done.


crates/prover/src/core/poly/circle/evaluation.rs line 168 at r3 (raw file):

Previously, shaharsamocha7 wrote…

Why not get the coset directly?
Also rename function and change doc accordingly

I need to order them according to the bit reverse order of the circle domain order, which is different than the canonic coset order. We could either

  1. Get the log size and compute the circle domain of the relevant canonic coset
  2. Get the canonic coset and then convert it to a circle domain
  3. Get a circle domain
    wdyt?

crates/prover/src/core/poly/circle/evaluation.rs line 168 at r3 (raw file):

Previously, shaharsamocha7 wrote…

Can we write here the simplest way to evaluate the barycentric_weights?

Done.

@Gali-StarkWare Gali-StarkWare force-pushed the gali/cpu_barycentric branch from 3f47b68 to 8dd5b33 Compare May 28, 2025 08:49
@Gali-StarkWare Gali-StarkWare marked this pull request as ready for review May 28, 2025 09:25
Copy link
Contributor

@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 2 files at r4, all commit messages.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @Alon-Ti and @Gali-StarkWare)


crates/prover/src/core/poly/circle/evaluation.rs line 168 at r3 (raw file):

Previously, Gali-StarkWare wrote…

I need to order them according to the bit reverse order of the circle domain order, which is different than the canonic coset order. We could either

  1. Get the log size and compute the circle domain of the relevant canonic coset
  2. Get the canonic coset and then convert it to a circle domain
  3. Get a circle domain
    wdyt?

I think getting CanonicCoset and use to_circle_domain explains it best, wdyt?

Copy link
Contributor

@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: 1 of 2 files reviewed, 4 unresolved discussions (waiting on @Alon-Ti and @Gali-StarkWare)


crates/prover/src/core/poly/circle/evaluation.rs line 198 at r5 (raw file):

            return weights;
        }
    }

Why do we need this? Let's make this function assert/assume that p is not in the domain
If it was, we could just return the evaluation and no need for weights, is that right?

Code quote:

    // If p is in the domain at position i, then w_j = δ_ij
    for i in 0..domain.size() {
        if domain.at(i).into_ef() == sample_point {
            let mut weights = vec![SecureField::zero(); domain.size()];
            weights[i] = SecureField::one();
            return weights;
        }
    }

crates/prover/src/core/poly/circle/evaluation.rs line 249 at r5 (raw file):

) -> SecureField {
    // Evaluation = Σ W_i * Poly(i) for all i in the evaluation domain.
    // For more information on barycentric weights calculation see [`weights`]

rename

Suggestion:

[`barycentric_weights`]

crates/prover/src/core/poly/circle/evaluation.rs line 254 at r5 (raw file):

            return evals.values[bit_reverse_index(i, evals.domain.log_size())].into();
        }
    }

I think this should be removed, but please comment or assert that p is not in domain

Code quote:

    for i in 0..evals.domain.size() {
        if point == evals.domain.at(i).into_ef() {
            return evals.values[bit_reverse_index(i, evals.domain.log_size())].into();
        }
    }

Copy link
Contributor Author

@Gali-StarkWare Gali-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: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @Alon-Ti and @shaharsamocha7)


crates/prover/src/core/poly/circle/evaluation.rs line 198 at r5 (raw file):

Previously, shaharsamocha7 wrote…

Why do we need this? Let's make this function assert/assume that p is not in the domain
If it was, we could just return the evaluation and no need for weights, is that right?

Done.


crates/prover/src/core/poly/circle/evaluation.rs line 249 at r5 (raw file):

Previously, shaharsamocha7 wrote…

rename

Done.


crates/prover/src/core/poly/circle/evaluation.rs line 254 at r5 (raw file):

Previously, shaharsamocha7 wrote…

I think this should be removed, but please comment or assert that p is not in domain

Done.

Copy link
Contributor

@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 r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Alon-Ti)


crates/prover/src/core/poly/circle/evaluation.rs line 192 at r6 (raw file):

    let domain = coset.circle_domain();

    let (s_i_i, v_i_p): (Vec<_>, Vec<_>) = (0..domain.size())

I think it is clearer

Suggestion:

 let (si_i, vi_p): (Vec<_>, Vec<_>) = (0..domain.size())

crates/prover/src/core/poly/circle/evaluation.rs line 195 at r6 (raw file):

        .map(|i| {
            let point_i = domain.at(i).into_ef::<SecureField>();
            let minus_two_times_point_i_y = point_i.y * SecureField::from(-2);

not sure about domain_point/coset_point, wdyt?
I'm also ok with just point

Suggestion:

            let domain_point = domain.at(i).into_ef::<SecureField>();
            let minus_two_point_y = point_i.y * SecureField::from(-2);

Copy link
Contributor Author

@Gali-StarkWare Gali-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: all files reviewed, 2 unresolved discussions (waiting on @Alon-Ti and @shaharsamocha7)


crates/prover/src/core/poly/circle/evaluation.rs line 192 at r6 (raw file):

Previously, shaharsamocha7 wrote…

I think it is clearer

Done also for vn_p


crates/prover/src/core/poly/circle/evaluation.rs line 195 at r6 (raw file):

Previously, shaharsamocha7 wrote…

not sure about domain_point/coset_point, wdyt?
I'm also ok with just point

Went with coset point

Copy link
Contributor

@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 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Alon-Ti)


crates/prover/src/core/poly/circle/evaluation.rs line 188 at r7 (raw file):

    //   [`coset_vanishing_derivative`].

    // TODO(Gali): Change weights order to bit-reverse order.

this todo is not in place (can fix in another pr)

Code quote:

    // TODO(Gali): Change weights order to bit-reverse order.

crates/prover/src/core/poly/circle/evaluation.rs line 303 at r7 (raw file):

            .iter()
            .map(|point| poly.eval_at_point(*point))
            .collect::<Vec<_>>();

unblocking

Suggestion:

.collect_vec();

@Gali-StarkWare Gali-StarkWare force-pushed the gali/cpu_barycentric branch from 09362b6 to 34fce22 Compare May 29, 2025 13:05
Copy link
Contributor Author

@Gali-StarkWare Gali-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 r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Alon-Ti)

Copy link
Contributor

@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 all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Alon-Ti)

@Gali-StarkWare Gali-StarkWare merged commit ad1902f into gali/barycentric Jun 5, 2025
19 of 20 checks passed
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