-
Notifications
You must be signed in to change notification settings - Fork 78
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
constant column attempt 1 #849
base: dev
Are you sure you want to change the base?
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @ohad-starkware and the rest of your teammates on Graphite |
1b63160
to
b2ba749
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #849 +/- ##
==========================================
- Coverage 91.86% 91.62% -0.24%
==========================================
Files 89 89
Lines 12080 12169 +89
Branches 12080 12169 +89
==========================================
+ Hits 11097 11150 +53
- Misses 876 908 +32
- Partials 107 111 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
885e6d7
to
5aa955e
Compare
5aa955e
to
26bb2a2
Compare
What about the other interactions? Code quote: let component_static_masks = &component.mask_points(point)[CONST_INTERACTION]; |
Previously, ilyalesokhin-starkware wrote…
look at the function above |
What is happening here? Code quote: .iter().filter(|l| l.col_start != l.col_end) |
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: 0 of 30 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)
crates/prover/src/core/pcs/utils.rs
line 130 at r4 (raw file):
Previously, ilyalesokhin-starkware wrote…
What is happening here?
sry this should have come with a TODO to handle it differently
it ignores 'empty' subspans , because the unwrap panics
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: 0 of 30 files reviewed, 2 unresolved discussions (waiting on @ohad-starkware)
crates/prover/src/constraint_framework/component.rs
line 182 at r4 (raw file):
[ trace.polys.sub_tree(&mask_spans), trace.polys.sub_tree_sparse(&self.static_columns_locations),
Why is this at the end?
Code quote:
trace.polys.sub_tree_sparse(&self.static_columns_locations),
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: 0 of 30 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)
crates/prover/src/constraint_framework/component.rs
line 182 at r4 (raw file):
Previously, ilyalesokhin-starkware wrote…
Why is this at the end?
order wont matter here -
Treevec::concat_cols is concat-by-index, meaning res[i] = concat (tree_0[i]..tree_n[i])
here there are 2 trees, the first has an empty vec in its first entry, and the second is empty in the rest.
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: 0 of 30 files reviewed, 3 unresolved discussions (waiting on @ohad-starkware)
crates/prover/src/core/air/components.rs
line 83 at r4 (raw file):
mask_values: &[Vec<SecureField>], ) -> ColumnVec<Vec<SecureField>> { let mask_by_column = &self.mask_points_by_column(point)[CONST_INTERACTION];
Why not?
Suggestion:
&self.const_mask_points_by_column(point);
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: 0 of 30 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware)
crates/prover/src/core/air/components.rs
line 83 at r4 (raw file):
Previously, ilyalesokhin-starkware wrote…
Why not?
I guess it was a mistake
iirc I tried to allow 'global access' regardless of the interaction, and later gave up on that.
This change is