-
Notifications
You must be signed in to change notification settings - Fork 91
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
Create MLE eval prover component #804
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #804 +/- ##
==========================================
+ Coverage 91.86% 92.05% +0.19%
==========================================
Files 89 89
Lines 12080 12098 +18
Branches 12080 12098 +18
==========================================
+ Hits 11097 11137 +40
+ Misses 876 854 -22
Partials 107 107
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
3a5da0f
to
c1ac882
Compare
af52728
to
1093cee
Compare
c1ac882
to
3fde6e5
Compare
1093cee
to
fd8ce1f
Compare
3fde6e5
to
bb56a26
Compare
fd8ce1f
to
71f60f3
Compare
110a7b8
to
6b4cd33
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 1 of 3 files at r2.
Reviewable status: 1 of 5 files reviewed, 1 unresolved discussion (waiting on @andrewmilson and @shaharsamocha7)
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 211 at r2 (raw file):
.values, ); let is_second_lde = VeryPackedBaseColumn::from(
Do we really need is_second? Can you not just take is_first at a mask offset, 1?
71f60f3
to
765f537
Compare
6b4cd33
to
7af83e4
Compare
765f537
to
9eb0dfa
Compare
7af83e4
to
3e7fdfa
Compare
9eb0dfa
to
4f77d82
Compare
3e7fdfa
to
4e0bed5
Compare
4f77d82
to
e67c1c5
Compare
4e0bed5
to
4953abd
Compare
e67c1c5
to
ce9c26a
Compare
4953abd
to
283d17a
Compare
ce9c26a
to
028665a
Compare
283d17a
to
62443c5
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: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7 and @spapinistarkware)
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 211 at r2 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Do we really need is_second? Can you not just take is_first at a mask offset, 1?
Updated to use the offset but honestly might be slower due to the bad access patterns.
Previously, andrewmilson (Andrew Milson) wrote…
Actually I'm wrong never mind |
028665a
to
7fd99fa
Compare
cb773df
to
d8ff784
Compare
7fd99fa
to
b58bab4
Compare
d8ff784
to
2dcf095
Compare
b58bab4
to
1e9e342
Compare
2dcf095
to
f0d2917
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 4 of 4 files at r5, all commit messages.
Reviewable status: 4 of 5 files reviewed, 5 unresolved discussions (waiting on @andrewmilson and @spapinistarkware)
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 68 at r5 (raw file):
impl<'twiddles, 'oracle, O: MleCoeffColumnOracle> MleEvalProverComponent<'twiddles, 'oracle, O> { // TODO(andrew): Some eval points may affect completeness. Document.
Can you document that?
Code quote:
// TODO(andrew): Some eval points may affect completeness. Document.
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 89 at r5 (raw file):
let trace_structure = mle_eval_info(interaction, n_variables).mask_offsets; let trace_locations = location_allocator.next_for_structure(&trace_structure);
Does TreeSubspan also useful for the constant columns?
Code quote:
let trace_locations = location_allocator.next_for_structure(&trace_structure);
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 172 at r5 (raw file):
carry_quotients_col_eval, is_first, is_second,
Should we get those as arguments?
Can we just get the constant column and then extract those values inside?
Code quote:
is_first,
is_second,
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 1009 at r5 (raw file):
let log_size = mle.n_variables() as u32; let trace_domain = CanonicCoset::new(log_size).circle_domain(); let mle_coeffs_col_by_coords = mle.clone().into_evals().into_secure_column_by_coords();
Is this copy the entire column? or what?
Code quote:
.clone().
f0d2917
to
8f1ab06
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: 3 of 5 files reviewed, 5 unresolved discussions (waiting on @shaharsamocha7 and @spapinistarkware)
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 68 at r5 (raw file):
Previously, shaharsamocha7 wrote…
Can you document that?
Done.
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 89 at r5 (raw file):
Previously, shaharsamocha7 wrote…
Does TreeSubspan also useful for the constant columns?
It can as constant columns are now.
It treats the constant interaction tree just like any interaction tree.
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 172 at r5 (raw file):
Previously, shaharsamocha7 wrote…
Should we get those as arguments?
Can we just get the constant column and then extract those values inside?
Added a TODO
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 1009 at r5 (raw file):
Previously, shaharsamocha7 wrote…
Is this copy the entire column? or what?
Yeah, it's just a test function
8f1ab06
to
5b3001a
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 1 of 2 files at r3, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)
This change is