-
Notifications
You must be signed in to change notification settings - Fork 85
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
Conversation
Codecov ReportAttention: Patch coverage is
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. |
338e26f
to
5596e74
Compare
d98224c
to
82d4bc3
Compare
82d4bc3
to
e0a07ca
Compare
5596e74
to
573ee79
Compare
a05370e
to
5f14eeb
Compare
573ee79
to
22ae396
Compare
5f14eeb
to
4e0f5c8
Compare
22ae396
to
d2f052c
Compare
4e0f5c8
to
b214af4
Compare
There was a problem hiding this 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?
d2f052c
to
1ac18ac
Compare
b214af4
to
b6c1cf1
Compare
There was a problem hiding this 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.
b6c1cf1
to
12d4559
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
1ac18ac
to
8cc4e98
Compare
12d4559
to
b02d634
Compare
8cc4e98
to
f0349c6
Compare
b02d634
to
ffd49fd
Compare
f0349c6
to
0f648e0
Compare
ffd49fd
to
345da9a
Compare
0f648e0
to
dc03192
Compare
9afd058
to
8053528
Compare
There was a problem hiding this 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
There was a problem hiding this 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: complete! all files reviewed, all discussions resolved (waiting on @shaharsamocha7)
dc03192
to
1c00ec1
Compare
8053528
to
bc7f1e0
Compare
This change is