CommitmentScheme utils

[spapinistarkware]

This change is 

[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

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

Project coverage is 94.67%. Comparing base (c324101) to head (fc18b4d).

Files	Patch %	Lines
src/core/commitment_scheme/utils.rs	0.00%	61 Missing ⚠️
src/core/mod.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@                           Coverage Diff                            @@
##           spapini/03-15-air_with_dyn_component     #500      +/-   ##
========================================================================
- Coverage                                 95.36%   94.67%   -0.69%     
========================================================================
  Files                                        58       59       +1     
  Lines                                      8795     8859      +64     
  Branches                                   8795     8859      +64     
========================================================================
  Hits                                       8387     8387              
- Misses                                      359      423      +64     
  Partials                                     49       49              

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

[alonh5]

Reviewed 2 of 4 files at r1, all commit messages.
Reviewable status: 2 of 5 files reviewed, 12 unresolved discussions (waiting on @spapinistarkware)

---

src/core/air/component_visitors.rs line 86 at r1 (raw file):

                let trace = self.component_traces.next().unwrap();
                let (points, values) = component.mask_points_and_values(self.point, trace);
                self.component_points.0.push(points);

You should be able to remove these throughout the PR.

Code quote:

.0

---

src/core/air/component_visitors.rs line 205 at r2 (raw file):

    }

    /// Returns the log degree bounds of the quotient polynomials in descending order.

Can you please move this comment to the function above? This is a mistake I made in a previous PR.

Code quote:

/// Returns the log degree bounds of the quotient polynomials in descending order.

---

src/core/air/component_visitors.rs line 206 at r2 (raw file):

    /// Returns the log degree bounds of the quotient polynomials in descending order.
    fn column_log_sizes(&self) -> Vec<u32> {

Why this change?

---

src/core/air/component_visitors.rs line 207 at r2 (raw file):

    /// Returns the log degree bounds of the quotient polynomials in descending order.
    fn column_log_sizes(&self) -> Vec<u32> {
        struct DomainsVisitor {

Please rename.

Code quote:

DomainsVisitor

---

src/core/commitment_scheme/utils.rs line 11 at r2 (raw file):

#[derive(Debug, Clone)]
pub struct TreeVec<T>(pub Vec<T>);
impl<T> TreeVec<T> {

Are all of these methods needed and used?

---

src/core/commitment_scheme/utils.rs line 95 at r2 (raw file):

    }
    /// Flattens the [TreeColumns] into a single [ColumnVec] with all the columns combined.
    pub fn flatten(self) -> ColumnVec<T> {

Then it's just a Vec.

Suggestion:

Vec

---

src/core/prover/mod.rs line 178 at r2 (raw file):

        .collect_vec();
    commitment_domains.push(CanonicCoset::new(
        air.max_constraint_log_degree_bound() + LOG_BLOWUP_FACTOR,

We should rename this right? The current name is more suiting for components.

Suggestion:

composition_polynomial_log_degree_bound

---

src/core/prover/utils.rs line 1 at r2 (raw file):

use crate::core::air::{Air, Component, ComponentVisitor};

What should both of these functions be doing? ATM they are not symmetrical IIUC. The first takes a ComponentVec and turns it into a TreeColumns containing a single tree, and the second can take a TreeColumns containing multiple trees and turns it into a single ComponentVec.

Also I think we should add unit tests for these.

---

src/core/prover/utils.rs line 7 at r2 (raw file):

pub fn component_wise_to_tree_wise<T>(
    _air: &impl Air<CPUBackend>,

Why keep this arg?

Code quote:

    _air: &impl Air<CPUBackend>,

---

src/core/prover/utils.rs line 26 at r2 (raw file):

            self.by_component
                .0
                .push(self.by_tree.take(component.mask().len()).collect());

Can you make sure this is mutating self.by_tree? In other places Io had to do (&mut self.by_tree).take.

Code quote:

self.by_tree.take

---

src/core/prover/utils.rs line 26 at r2 (raw file):

            self.by_component
                .0
                .push(self.by_tree.take(component.mask().len()).collect());

Shouldn't you be taking the number of columns and not the number of mask items?

Code quote:

component.mask().len()

---

src/core/prover/utils.rs line 29 at r2 (raw file):

        }
    }
    let values = values.0.remove(0);

Can you explain please?

Code quote:

let values = values.0.remove(0);

[spapinistarkware]

Reviewable status: 1 of 9 files reviewed, 7 unresolved discussions (waiting on @alonh5)

---

src/core/commitment_scheme/utils.rs line 11 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Are all of these methods needed and used?

Yes.

---

src/core/commitment_scheme/utils.rs line 95 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Then it's just a Vec.

Why just a vec? It's an element per column.

---

src/core/air/component_visitors.rs line 86 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

You should be able to remove these throughout the PR.

Done.

---

src/core/air/component_visitors.rs line 205 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Can you please move this comment to the function above? This is a mistake I made in a previous PR.

Done.

---

src/core/air/component_visitors.rs line 207 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Please rename.

Still relevant?

---

src/core/prover/utils.rs line 1 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

What should both of these functions be doing? ATM they are not symmetrical IIUC. The first takes a ComponentVec and turns it into a TreeColumns containing a single tree, and the second can take a TreeColumns containing multiple trees and turns it into a single ComponentVec.

Also I think we should add unit tests for these.

Done.

---

src/core/prover/utils.rs line 7 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Why keep this arg?

Done.

[alonh5]

Reviewable status: 1 of 9 files reviewed, 3 unresolved discussions (waiting on @spapinistarkware)

---

src/core/commitment_scheme/utils.rs line 56 at r4 (raw file):

/// A container that holds an element for each column of each commitment tree.
#[derive(Debug, Clone)]
pub struct TreeColumns<T>(pub TreeVec<ColumnVec<T>>);

Why do you need this wrapper type? Isn't it enough to use TreeVec<ColumnVec>?

[alonh5]

Reviewable status: 1 of 9 files reviewed, 3 unresolved discussions (waiting on @spapinistarkware)

---

src/core/prover/utils.rs line 1 at r2 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Done.

To which branch did these move?

[spapinistarkware]

Reviewable status: 1 of 9 files reviewed, 3 unresolved discussions (waiting on @alonh5)

---

src/core/commitment_scheme/utils.rs line 56 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Why do you need this wrapper type? Isn't it enough to use TreeVec<ColumnVec>?

Done.

---

src/core/prover/utils.rs line 1 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

To which branch did these move?

They were eliminated, I didnt want them.

[alonh5]

Reviewed 5 of 7 files at r4, 2 of 3 files at r5, all commit messages.
Reviewable status: 8 of 9 files reviewed, 1 unresolved discussion

[alonh5]

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

---

src/core/commitment_scheme/utils.rs line 1 at r5 (raw file):

use std::iter::zip;

Please add blank lines between code blocks.

---

src/core/commitment_scheme/utils.rs line 16 at r5 (raw file):

    }
    pub fn map<U, F: Fn(T) -> U>(self, f: F) -> TreeVec<U> {
        TreeVec(self.0.into_iter().map(f).collect())

Can you remove all of these?

Code quote:

.0

---

src/core/commitment_scheme/utils.rs line 27 at r5 (raw file):

    }
}
// Converts &TreeVec<T> to TreeVec<&T>.

Suggestion:

/// Converts &TreeVec<T> to TreeVec<&T>.

---

src/core/commitment_scheme/utils.rs line 67 at r5 (raw file):

    ) -> TreeVec<ColumnVec<(T, U)>> {
        let other = other.into();
        assert_eq!(self.0.len(), other.0.len());

Maybe also use zip_eq?

Code quote:

assert_eq!(self.0.len(), other.0.len());

---

src/core/commitment_scheme/utils.rs line 84 at r5 (raw file):

        )
    }
    /// Flattens the [TreeColumns] into a single [ColumnVec] with all the columns combined.

Same below.

Suggestion:

/// Flattens the [TreeVec]

---

src/core/commitment_scheme/utils.rs line 97 at r5 (raw file):

    /// Flattens a [TreeColumns] of [Vec]s into a single [Vec] with all the elements combined.
    pub fn flatten_all(self) -> Vec<T> {
        self.flatten().into_iter().flatten().collect()

The intermediate collect() is optimized out right?

Code quote:

self.flatten().into_iter().flatten().collect()

[spapinistarkware]

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

---

src/core/commitment_scheme/utils.rs line 1 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Please add blank lines between code blocks.

Done.

---

src/core/commitment_scheme/utils.rs line 16 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Can you remove all of these?

Removed wherever I could

---

src/core/commitment_scheme/utils.rs line 27 at r5 (raw file):

    }
}
// Converts &TreeVec<T> to TreeVec<&T>.

Done.

---

src/core/commitment_scheme/utils.rs line 84 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Same below.

Done.

---

src/core/commitment_scheme/utils.rs line 97 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

The intermediate collect() is optimized out right?

Not sure what you mean. There is one collect at the end, and it's not optimized out. It builds a new vec. But there are no clones of T here.

[alonh5]

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

---

src/core/commitment_scheme/utils.rs line 1 at r5 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Done.

Also between functions? That's how I see it is in other crates.

---

src/core/commitment_scheme/utils.rs line 97 at r5 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Not sure what you mean. There is one collect at the end, and it's not optimized out. It builds a new vec. But there are no clones of T here.

I mean the first flatten() iterates and collects, and then you iterate and collect again. The alternative would be this:

self.into_iter().flatten().flatten().collect()

---

src/core/commitment_scheme/utils.rs line 72 at r6 (raw file):

    ) -> TreeVec<ColumnVec<(T, U)>> {
        let other = other.into();
        assert_eq!(self.len(), other.len());

You can remove this now with the zip_eq?
Same in TreeVec::zip().

Code quote:

assert_eq!(self.len(), other.len());

[spapinistarkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5)

---

src/core/commitment_scheme/utils.rs line 1 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Also between functions? That's how I see it is in other crates.

Nah, I think it's good as it is.

---

src/core/commitment_scheme/utils.rs line 97 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

I mean the first flatten() iterates and collects, and then you iterate and collect again. The alternative would be this:

self.into_iter().flatten().flatten().collect()

Got it. Changed.

---

src/core/commitment_scheme/utils.rs line 72 at r6 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

You can remove this now with the zip_eq?
Same in TreeVec::zip().

Done.

[alonh5]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)

---

src/core/commitment_scheme/utils.rs line 22 at r7 (raw file):

        let other = other.into();
        assert_eq!(self.len(), other.len());
        TreeVec(zip(self.0, other.0).collect())

Suggestion:

        TreeVec(zip_eq(self.0, other.0).collect())

[alonh5]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @spapinistarkware)

[spapinistarkware]

Merge activity

• Mar 19, 11:10 AM EDT: @spapinistarkware started a stack merge that includes this pull request via Graphite.
• Mar 19, 11:13 AM EDT: Graphite rebased this pull request as part of a merge.
• Mar 19, 11:17 AM EDT: @spapinistarkware merged this pull request with Graphite.