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

Pass entire mask to components #801

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

andrewmilson
Copy link
Contributor

@andrewmilson andrewmilson commented Aug 22, 2024

This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 92.18%. Comparing base (1003bf2) to head (bc7f1e0).
Report is 1 commits behind head on dev.

Files Patch % Lines
crates/prover/src/core/prover/mod.rs 74.07% 2 Missing and 5 partials ⚠️
...rates/prover/src/constraint_framework/component.rs 94.23% 1 Missing and 2 partials ⚠️
crates/prover/src/core/pcs/utils.rs 88.23% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #801      +/-   ##
==========================================
- Coverage   92.25%   92.18%   -0.08%     
==========================================
  Files          91       90       -1     
  Lines       12484    12473      -11     
  Branches    12484    12473      -11     
==========================================
- Hits        11517    11498      -19     
- Misses        861      864       +3     
- Partials      106      111       +5     

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

@andrewmilson andrewmilson force-pushed the 08-20-Add_MleCollection_for_accumulating_MLEs branch from 338e26f to 5596e74 Compare August 23, 2024 23:39
@andrewmilson andrewmilson force-pushed the 08-21-Pass_entire_mask_to_components branch 2 times, most recently from d98224c to 82d4bc3 Compare August 23, 2024 23:46
@andrewmilson andrewmilson force-pushed the 08-21-Pass_entire_mask_to_components branch from 82d4bc3 to e0a07ca Compare August 24, 2024 03:28
@andrewmilson andrewmilson force-pushed the 08-20-Add_MleCollection_for_accumulating_MLEs branch from 5596e74 to 573ee79 Compare August 24, 2024 03:30
@andrewmilson andrewmilson force-pushed the 08-21-Pass_entire_mask_to_components branch 3 times, most recently from a05370e to 5f14eeb Compare August 24, 2024 04:35
@andrewmilson andrewmilson marked this pull request as ready for review August 24, 2024 23:54
@andrewmilson andrewmilson force-pushed the 08-20-Add_MleCollection_for_accumulating_MLEs branch from 573ee79 to 22ae396 Compare August 26, 2024 04:15
@andrewmilson andrewmilson force-pushed the 08-21-Pass_entire_mask_to_components branch from 5f14eeb to 4e0f5c8 Compare August 26, 2024 04:15
@andrewmilson andrewmilson force-pushed the 08-20-Add_MleCollection_for_accumulating_MLEs branch from 22ae396 to d2f052c Compare August 26, 2024 04:18
@andrewmilson andrewmilson force-pushed the 08-21-Pass_entire_mask_to_components branch from 4e0f5c8 to b214af4 Compare August 26, 2024 04:18
Copy link
Contributor

@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.

Reviewed 20 of 20 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)


crates/prover/src/constraint_framework/component.rs line 28 at r2 (raw file):

// TODO(andrew): Consider better location for this.
#[derive(Debug, Default)]
pub struct TreeColumnSpanProvider {

WDYT?
And maybe We should rename TreeColumnSpan to TraceLocation?

Suggestion:

TraceLocationAllocator

crates/prover/src/constraint_framework/component.rs line 30 at r2 (raw file):

pub struct TreeColumnSpanProvider {
    /// Mapping of tree index to next available column offset.
    next_tree_offsets: BTreeMap<usize, usize>,

Why not TreeVec?

Suggestion:

TreeVec<usize>

crates/prover/src/constraint_framework/component.rs line 35 at r2 (raw file):

impl TreeColumnSpanProvider {
    fn next_for_structure<T>(&mut self, structure: &TreeVec<ColumnVec<T>>) -> Vec<TreeColumnSpan> {
        structure
    When next_tree_offsets is TreeVec:

Suggestion:

impl TreeColumnSpanProvider {
    fn next_for_structure<T>(&mut self, structure: &TreeVec<ColumnVec<T>>) -> TreeVec<TreeColumnSpan> {
        self.next_tree_offsets.resize(...)
        self.next_tree_offsets.as_mut().zip(structure).map(|(off, cols)|{
        ...
        })
`

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

    let commitment_scheme_proof = commitment_scheme.prove_values(sample_points, channel);

    // TODO(spapini): Save clone.

I don't think there's a clone here, right? Can you remove?


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

    let mut composition_cols = mask.last().into_iter().flatten();

    let col0 = &**composition_cols.next().ok_or(InvalidOodsSampleStructure)?;

Can you do this with std::array::from_fn?

@andrewmilson andrewmilson force-pushed the 08-20-Add_MleCollection_for_accumulating_MLEs branch from d2f052c to 1ac18ac Compare August 26, 2024 16:18
@andrewmilson andrewmilson force-pushed the 08-21-Pass_entire_mask_to_components branch from b214af4 to b6c1cf1 Compare August 26, 2024 16:18
Copy link
Contributor Author

@andrewmilson andrewmilson 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, 5 unresolved discussions (waiting on @shaharsamocha7 and @spapinistarkware)


crates/prover/src/constraint_framework/component.rs line 28 at r2 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

WDYT?
And maybe We should rename TreeColumnSpan to TraceLocation?

Updated. Personally prefer TreeColumnSpan since the trace location is made up of multiple WDYT?


crates/prover/src/constraint_framework/component.rs line 30 at r2 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Why not TreeVec?

Done.


crates/prover/src/constraint_framework/component.rs line 35 at r2 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

When next_tree_offsets is TreeVec:

Done. Seemed simpler to use regular iterators though because of needing the index.


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

Previously, spapinistarkware (Shahar Papini) wrote…

I don't think there's a clone here, right? Can you remove?

Done.


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

Previously, spapinistarkware (Shahar Papini) wrote…

Can you do this with std::array::from_fn?

Done.

@andrewmilson andrewmilson force-pushed the 08-21-Pass_entire_mask_to_components branch from b6c1cf1 to 12d4559 Compare August 26, 2024 16:26
Copy link
Contributor

@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.

:lgtm:

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


crates/prover/src/constraint_framework/component.rs line 28 at r2 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Updated. Personally prefer TreeColumnSpan since the trace location is made up of multiple WDYT?

I'm just afraid that new people looking at this code will think it's Jibrish...
I'm open to new names.
Maybe TraceSubspan ?


crates/prover/src/core/pcs/mod.rs line 28 at r3 (raw file):

}

// #[derive(Debug, Clone, PartialEq, Eq)]

Remove?

@andrewmilson andrewmilson force-pushed the 08-20-Add_MleCollection_for_accumulating_MLEs branch from 1ac18ac to 8cc4e98 Compare August 28, 2024 02:02
@andrewmilson andrewmilson force-pushed the 08-21-Pass_entire_mask_to_components branch from 12d4559 to b02d634 Compare August 28, 2024 02:02
@andrewmilson andrewmilson force-pushed the 08-20-Add_MleCollection_for_accumulating_MLEs branch from 8cc4e98 to f0349c6 Compare August 28, 2024 02:11
@andrewmilson andrewmilson force-pushed the 08-21-Pass_entire_mask_to_components branch from b02d634 to ffd49fd Compare August 28, 2024 02:11
@andrewmilson andrewmilson force-pushed the 08-20-Add_MleCollection_for_accumulating_MLEs branch from f0349c6 to 0f648e0 Compare August 28, 2024 02:18
@andrewmilson andrewmilson force-pushed the 08-21-Pass_entire_mask_to_components branch from ffd49fd to 345da9a Compare August 28, 2024 02:18
@andrewmilson andrewmilson force-pushed the 08-20-Add_MleCollection_for_accumulating_MLEs branch from 0f648e0 to dc03192 Compare August 28, 2024 03:07
@andrewmilson andrewmilson force-pushed the 08-21-Pass_entire_mask_to_components branch 2 times, most recently from 9afd058 to 8053528 Compare August 28, 2024 03:17
Copy link
Contributor Author

@andrewmilson andrewmilson 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: 16 of 22 files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7 and @spapinistarkware)


crates/prover/src/constraint_framework/component.rs line 28 at r2 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

I'm just afraid that new people looking at this code will think it's Jibrish...
I'm open to new names.
Maybe TraceSubspan ?

Done


crates/prover/src/core/pcs/mod.rs line 28 at r3 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Remove?

Done

Copy link
Contributor

@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.

Reviewed 6 of 6 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @shaharsamocha7)

@andrewmilson andrewmilson force-pushed the 08-20-Add_MleCollection_for_accumulating_MLEs branch from dc03192 to 1c00ec1 Compare August 28, 2024 13:00
Base automatically changed from 08-20-Add_MleCollection_for_accumulating_MLEs to dev August 28, 2024 13:03
@andrewmilson andrewmilson force-pushed the 08-21-Pass_entire_mask_to_components branch from 8053528 to bc7f1e0 Compare August 28, 2024 13:03
@andrewmilson andrewmilson merged commit e3858fb into dev Aug 28, 2024
16 checks passed
@andrewmilson andrewmilson deleted the 08-21-Pass_entire_mask_to_components branch August 28, 2024 13:08
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