-
Notifications
You must be signed in to change notification settings - Fork 82
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
Combine eq step constraints into single constraint #799
Combine eq step constraints into single constraint #799
Conversation
0d04479
to
73f1ab4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #799 +/- ##
==========================================
+ Coverage 92.26% 92.35% +0.09%
==========================================
Files 91 91
Lines 12442 12481 +39
Branches 12442 12481 +39
==========================================
+ Hits 11479 11527 +48
+ Misses 857 848 -9
Partials 106 106 ☔ View full report in Codecov by Sentry. |
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 all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @andrewmilson)
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 86 at r1 (raw file):
// Check all variables except the last (last variable is handled by the constraint above). let step_constraints = (0..N_VARIABLES.saturating_sub(1)).map(|variable_i| {
Combine the constraints like we discussed
e0f1063
to
41485b5
Compare
03fd53c
to
740441b
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 2 files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 86 at r1 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Combine the constraints like we discussed
Done.
486c573
to
42f2f6f
Compare
8c909c4
to
efe945e
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 r2, 1 of 2 files at r3, all commit messages.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 105 at r3 (raw file):
// Check all the steps. eval.add_constraint(curr - next_next * carry_quotients_col_eval);
carry_quotients_col_eval
can't be a constant column, right? I woudl imagine you need to evaluate it at a point, but eval_carry_quotient_col doesn't seem to accept EvalAtRow, so, how does carry_quotients_col_eval
this get here?
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 213 at r3 (raw file):
let step_coset = Coset::new(coset.initial_index, coset.log_size - 1); let mut res = (coset_vanishing(coset, p) / coset_vanishing(step_coset, p)).square() / BaseField::from(2);
Division is a bummer. Especially when you compute it log_step times!
I think you can compute all of these it in a better way.
The straight forward way is to do a batch inverse. But you canprobably save a bit more, if u do it wisely.
efe945e
to
97e84fa
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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @shaharsamocha7 and @spapinistarkware)
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 105 at r3 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
carry_quotients_col_eval
can't be a constant column, right? I woudl imagine you need to evaluate it at a point, but eval_carry_quotient_col doesn't seem to accept EvalAtRow, so, how doescarry_quotients_col_eval
this get here?
Done. Discussed offline.
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 213 at r3 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Division is a bummer. Especially when you compute it log_step times!
I think you can compute all of these it in a better way.
The straight forward way is to do a batch inverse. But you canprobably save a bit more, if u do it wisely.
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 all commit messages.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 217 at r4 (raw file):
let mut numers = vec![coset_vanishing(coset, p).square()]; let mut denoms = vec![coset_vanishing(step_coset, p).square().double()]; for log_sub_step in 1..log_step {
Consider something like this:
let p_doublings = (0..log).scan(p,|(p,_)| (p.double(),p)).collect();
let initial_doublings = (0..log).scan(initial_point,|(p,_)| (p.double(),p)).collect();
let selectors = p_doublings.zip(initial_doublings).rev().scan(
|(last_sel,(q,c))| (last_sel*(q.x-c.y))
);
}
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 219 at r4 (raw file):
for log_sub_step in 1..log_step { let step_coset = Coset::new(coset.initial_index, coset.log_size - log_sub_step); numers.push(coset_vanishing(coset, p));
Isn't this the same expression at every iteration?
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: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 182 at r4 (raw file):
let log_step = variable_i as u32 + 2; let offset = (1 << (log_step - 1)) - 2; let half_coset0_selector = eval_step_selector_with_offset(coset, offset, log_step, p);
Now i'm not so sure why this works...
The selector you compute. Is it all 1s and 0s, or is it just 0s and non 0s?
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: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @shaharsamocha7 and @spapinistarkware)
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 182 at r4 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Now i'm not so sure why this works...
The selector you compute. Is it all 1s and 0s, or is it just 0s and non 0s?
All 1s and 0s
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 219 at r4 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Isn't this the same expression at every iteration?
Yes the num
2d6cc30
to
8b81736
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: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @shaharsamocha7 and @spapinistarkware)
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 182 at r4 (raw file):
Previously, andrewmilson (Andrew Milson) wrote…
All 1s and 0s
start with coset_vanish(coset, p).square() / coset_vanish(half_coset, p) gives you all 1s on half coset0
Normalise and return there if we want step 2
Then add coset_vanish(coset, p)/coset_vanish(half_coset, p) which alternates 1, -1 on half coset0 and 0 on
Normalise and return there if we want step 4
Then add coset_vanish(coset, p)/coset_vanish(half_half_coset, p) which alternates 2, 0, -2, 0 on half coset0
Normalise and return there if we want step 8
etc.
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 217 at r4 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Consider something like this:
let p_doublings = (0..log).scan(p,|(p,_)| (p.double(),p)).collect(); let initial_doublings = (0..log).scan(initial_point,|(p,_)| (p.double(),p)).collect(); let selectors = p_doublings.zip(initial_doublings).rev().scan( |(last_sel,(q,c))| (last_sel*(q.x-c.y)) ); }
Think we need to do the quotienting to get 0 or 1 selectors on coset.
Maybe I don't fully understand the technique you're proposing.
I made some similarish modifications though WDYT?
crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs
line 219 at r4 (raw file):
Previously, andrewmilson (Andrew Milson) wrote…
Yes the num
Done
8b81736
to
a4dc0be
Compare
a4dc0be
to
c3af357
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 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @shaharsamocha7)
This change is