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

Combine eq step constraints into single constraint #799

Merged

Conversation

andrewmilson
Copy link
Contributor

@andrewmilson andrewmilson commented Aug 21, 2024

This change is Reviewable

@andrewmilson andrewmilson force-pushed the 08-21-Combine_eq_step_constraints_into_single_constraint branch from 0d04479 to 73f1ab4 Compare August 21, 2024 19:08
@codecov-commenter
Copy link

codecov-commenter commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.35%. Comparing base (f16ba08) to head (c3af357).
Report is 4 commits behind head on dev.

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.
📢 Have feedback on the report? Share it here.

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

@andrewmilson andrewmilson force-pushed the 08-20-Move_all_MLE_eval_related_constraints_into_same_module branch from e0f1063 to 41485b5 Compare August 22, 2024 14:38
Base automatically changed from 08-20-Move_all_MLE_eval_related_constraints_into_same_module to dev August 22, 2024 14:42
@andrewmilson andrewmilson force-pushed the 08-21-Combine_eq_step_constraints_into_single_constraint branch 2 times, most recently from 03fd53c to 740441b Compare August 22, 2024 17:26
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: 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.

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

@andrewmilson andrewmilson force-pushed the 08-21-Combine_eq_step_constraints_into_single_constraint branch from efe945e to 97e84fa Compare August 26, 2024 15:24
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: 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 does carry_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.

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 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?

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.

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?

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: 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

@andrewmilson andrewmilson force-pushed the 08-21-Combine_eq_step_constraints_into_single_constraint branch 2 times, most recently from 2d6cc30 to 8b81736 Compare August 28, 2024 02:11
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: 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

@andrewmilson andrewmilson force-pushed the 08-21-Combine_eq_step_constraints_into_single_constraint branch from 8b81736 to a4dc0be Compare August 28, 2024 02:18
@andrewmilson andrewmilson force-pushed the 08-21-Combine_eq_step_constraints_into_single_constraint branch from a4dc0be to c3af357 Compare August 28, 2024 03:07
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 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @shaharsamocha7)

@andrewmilson andrewmilson merged commit 793a498 into dev Aug 28, 2024
16 checks passed
@andrewmilson andrewmilson deleted the 08-21-Combine_eq_step_constraints_into_single_constraint branch August 28, 2024 12:57
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