QuotientOps

[spapinistarkware]

This change is 

[graphite-app]

Your org has enabled the Graphite merge queue for merging into dev

Add the label “merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

[spapinistarkware]

• parallel wide fib test #511
• Evaluation oods once #496
• Reuse commitment evaluation #495
• Use precomputed twiddles #494
• Add measurement logs #493
• WideFib test with AVX Backend #492
• Implement Backend for AVX512Backend #491
• Make prove() generic in Backend #490
• CommitmentSchemeProver generic in backend #489
• AVX quotients #488
• Clean up fri quotients #486
• Save inverse on quotient numerator #484
• Commitment Scheme evaluation per size #483
• Separate commitment prover and verifier to modules #482
• QuotientOps #481 👈
• DomainAccumulator allows accumulating a row at once #480
• Rename evalaution.rs to accumulaion.rs #479
• Move SecureColumn #478
• Use only canonic domains #463
• Revert "integrated batch inverse in fib cpu (#448)" #477
• FRI in Commitment Scheme #476
• Field debug implementations #501
• CommitmentScheme utils #500
• Air with dyn component #487
• dev

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

Join @spapinistarkware and the rest of your teammates on Graphite

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.09%. Comparing base (760ccc2) to head (229a2f9).

Additional details and impacted files

@@                                          Coverage Diff                                          @@
##           spapini/03-15-domainaccumulator_allows_accumulating_a_row_at_once     #481      +/-   ##
=====================================================================================================
+ Coverage                                                              95.06%   95.09%   +0.03%     
=====================================================================================================
  Files                                                                     61       62       +1     
  Lines                                                                   9130     9195      +65     
  Branches                                                                9130     9195      +65     
=====================================================================================================
+ Hits                                                                    8679     8744      +65     
  Misses                                                                   393      393              
  Partials                                                                  58       58              

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

[alonh5]

Reviewed 3 of 4 files at r1, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @andrewmilson and @spapinistarkware)

---

src/core/backend/cpu/quotients.rs line 17 at r4 (raw file):

    fn accumulate_quotients(
        domain: CircleDomain,
        mut accum: ColumnAccumulator<'_, Self>,

Shouldn't it be by reference?

Suggestion:

accum: &mut ColumnAccumulator<'_, Self>,

---

src/core/backend/cpu/quotients.rs line 30 at r4 (raw file):

                    let column = &columns[*column_index];
                    let value = column[row];
                    numerator = numerator * random_coeff + (value - *open_value);

This should be a line between the value and it's complex conjugate.

Code quote:

*open_value

---

src/core/backend/cpu/quotients.rs line 39 at r4 (raw file):

                );

                row_accumlator *= random_coeff.pow(opening.column_indices_and_values.len() as u128)

This should also be fixed.

Code quote:

opening.column_indices_and_values.len() as u128

---

src/core/commitment_scheme/quotients.rs line 9 at r4 (raw file):

pub trait QuotientOps: Backend {
    fn accumulate_quotients(

Document.

Code quote:

fn accumulate_quotients(

---

src/core/commitment_scheme/quotients.rs line 18 at r4 (raw file):

}

pub struct BatchedColumnOpenings {

Document.

---

src/core/commitment_scheme/quotients.rs line 18 at r4 (raw file):

}

pub struct BatchedColumnOpenings {

Can we change all open/opening to oods? opening is overloaded.

Suggestion:

BatchedColumnOods

[spapinistarkware]

Reviewable status: 1 of 4 files reviewed, 4 unresolved discussions (waiting on @alonh5 and @andrewmilson)

---

src/core/backend/cpu/quotients.rs line 30 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

This should be a line between the value and it's complex conjugate.

Done.

---

src/core/backend/cpu/quotients.rs line 39 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

This should also be fixed.

Done.

---

src/core/commitment_scheme/quotients.rs line 9 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Document.

Done.

---

src/core/commitment_scheme/quotients.rs line 18 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Document.

Done.

[alonh5]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @andrewmilson and @spapinistarkware)

---

src/core/backend/cpu/quotients.rs line 24 at r5 (raw file):

        let mut res = SecureColumn::zeros(domain.size());
        for row in 0..domain.size() {
            let domain_point = domain.at(bit_reverse_index(row, domain.log_size()));

Not for this PR but do you think we should have a more efficient way to iterate over the domain in bit reverse order? Maybe also store an array of points in the domain?

Code quote:

let domain_point = domain.at(bit_reverse_index(row, domain.log_size()));

---

src/core/backend/cpu/quotients.rs line 25 at r5 (raw file):

        for row in 0..domain.size() {
            let domain_point = domain.at(bit_reverse_index(row, domain.log_size()));
            let row_accumlator =

Suggestion:

row_value

---

src/core/backend/cpu/quotients.rs line 91 at r5 (raw file):

        );
        let quot_poly_base_field =
            CPUCircleEvaluation::new(eval_domain, quot_eval.columns[0].clone()).interpolate();

Why are you only testing the first base filed column is in the fft space?

Code quote:

quot_eval.columns[0]

---

src/core/commitment_scheme/quotients.rs line 11 at r5 (raw file):

pub trait QuotientOps: Backend {
    /// Accumulates the quotients of the columns at the given domain.
    /// For a column f(x), and a point opening (p,v), the quotient is

Suggestion:

sample/sampling

---

src/core/commitment_scheme/quotients.rs line 20 at r5 (raw file):

        columns: &[&CircleEvaluation<Self, BaseField, BitReversedOrder>],
        random_coeff: SecureField,
        openings: &[ColumnSampleBatch],

Suggestion:

samples/samplings

---

src/core/commitment_scheme/quotients.rs line 26 at r5 (raw file):

/// A batch of column samplings at a point.
pub struct ColumnSampleBatch {
    /// The point at which the columns are opened.

Suggestion:

sampled.

[spapinistarkware]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alonh5 and @andrewmilson)

---

src/core/backend/cpu/quotients.rs line 24 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Not for this PR but do you think we should have a more efficient way to iterate over the domain in bit reverse order? Maybe also store an array of points in the domain?

We can, it's not super complicated.

---

src/core/backend/cpu/quotients.rs line 91 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Why are you only testing the first base filed column is in the fft space?

Good enough fot the test

---

src/core/commitment_scheme/quotients.rs line 11 at r5 (raw file):

pub trait QuotientOps: Backend {
    /// Accumulates the quotients of the columns at the given domain.
    /// For a column f(x), and a point opening (p,v), the quotient is

Done.

---

src/core/commitment_scheme/quotients.rs line 20 at r5 (raw file):

        columns: &[&CircleEvaluation<Self, BaseField, BitReversedOrder>],
        random_coeff: SecureField,
        openings: &[ColumnSampleBatch],

Done.

---

src/core/commitment_scheme/quotients.rs line 26 at r5 (raw file):

/// A batch of column samplings at a point.
pub struct ColumnSampleBatch {
    /// The point at which the columns are opened.

Done.

[alonh5]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @andrewmilson and @spapinistarkware)

---

src/core/backend/cpu/quotients.rs line 24 at r5 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

We can, it's not super complicated.

Add a TODO? You can put it on me

---

src/core/backend/cpu/quotients.rs line 14 at r6 (raw file):

use crate::core::poly::BitReversedOrder;
use crate::core::utils::bit_reverse_index;

open(ing) -> sample in this file too.

[spapinistarkware]

Merge activity

• Mar 24, 9:16 AM EDT: @spapinistarkware started a stack merge that includes this pull request via Graphite.
• Mar 24, 9:19 AM EDT: Graphite rebased this pull request as part of a merge.
• Mar 24, 9:21 AM EDT: @spapinistarkware merged this pull request with Graphite.