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

Add build trace functions for MLE eval component #803

Merged

Conversation

andrewmilson
Copy link
Contributor

@andrewmilson andrewmilson commented Aug 24, 2024

This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.76%. Comparing base (8102837) to head (1e9e342).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #803   +/-   ##
=======================================
  Coverage   92.76%   92.76%           
=======================================
  Files          89       89           
  Lines       12032    12032           
  Branches    12032    12032           
=======================================
  Hits        11162    11162           
  Misses        763      763           
  Partials      107      107           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrewmilson andrewmilson force-pushed the 08-23-Remove_InteractionElements_and_LookupValues branch from 18b014c to 8b554c7 Compare August 26, 2024 04:15
@andrewmilson andrewmilson force-pushed the 08-24-Add_build_trace_functions_for_MLE_eval_component branch from af52728 to 1093cee Compare August 26, 2024 04:15
@andrewmilson andrewmilson force-pushed the 08-23-Remove_InteractionElements_and_LookupValues branch from 8b554c7 to b03472c Compare August 26, 2024 04:18
@andrewmilson andrewmilson force-pushed the 08-24-Add_build_trace_functions_for_MLE_eval_component branch from 1093cee to fd8ce1f Compare August 26, 2024 04:18
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 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @andrewmilson and @shaharsamocha7)


crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs line 156 at r1 (raw file):

    #[cfg(test)]
    assert_eq!(claim, mle.eval_at_point(eval_point));

Consider

Suggestion:

    debug_assert_eq!(claim, mle.eval_at_point(eval_point));

crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs line 159 at r1 (raw file):

    let shift = claim / BaseField::from(mle.len());
    let packed_shift_coords = PackedSecureField::broadcast(shift).into_packed_m31s();
    let mut shifted_mle_terms_cols = mle_terms_cols.clone();

Suggestion:

let mut shifted_mle_terms_cols = mle_terms_cols;

crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs line 188 at r1 (raw file):

) -> Vec<CircleEvaluation<SimdBackend, BaseField, BitReversedOrder>> {
    let log_size = N_VARIABLES as u32;
    vec![gen_is_first(log_size)]

Techinically, you can also compute this using vanishing cosets, same way you did for the smaller ones.

@andrewmilson andrewmilson force-pushed the 08-23-Remove_InteractionElements_and_LookupValues branch from b03472c to 2053bd2 Compare August 26, 2024 16:29
@andrewmilson andrewmilson force-pushed the 08-24-Add_build_trace_functions_for_MLE_eval_component branch from fd8ce1f to 71f60f3 Compare August 26, 2024 17:44
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 1 files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7 and @spapinistarkware)


crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs line 156 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Consider

need the cfg(test) since eval_at_point defined behind cfg(test)


crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs line 159 at r1 (raw file):

    let shift = claim / BaseField::from(mle.len());
    let packed_shift_coords = PackedSecureField::broadcast(shift).into_packed_m31s();
    let mut shifted_mle_terms_cols = mle_terms_cols.clone();

ty


crates/prover/src/examples/xor/gkr_lookups/mle_eval.rs line 188 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Techinically, you can also compute this using vanishing cosets, same way you did for the smaller ones.

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 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @shaharsamocha7)

@andrewmilson andrewmilson force-pushed the 08-23-Remove_InteractionElements_and_LookupValues branch from 2053bd2 to d6b9dca Compare August 28, 2024 02:02
@andrewmilson andrewmilson force-pushed the 08-24-Add_build_trace_functions_for_MLE_eval_component branch from 71f60f3 to 765f537 Compare August 28, 2024 02:02
@andrewmilson andrewmilson force-pushed the 08-23-Remove_InteractionElements_and_LookupValues branch from d6b9dca to 6c3daff Compare August 28, 2024 02:11
@andrewmilson andrewmilson force-pushed the 08-24-Add_build_trace_functions_for_MLE_eval_component branch from 765f537 to 9eb0dfa Compare August 28, 2024 02:11
@andrewmilson andrewmilson force-pushed the 08-23-Remove_InteractionElements_and_LookupValues branch from 6c3daff to c126b7a Compare August 28, 2024 02:19
@andrewmilson andrewmilson force-pushed the 08-24-Add_build_trace_functions_for_MLE_eval_component branch from 9eb0dfa to 4f77d82 Compare August 28, 2024 02:19
@andrewmilson andrewmilson force-pushed the 08-23-Remove_InteractionElements_and_LookupValues branch from c126b7a to c6bfa66 Compare August 28, 2024 03:07
@andrewmilson andrewmilson force-pushed the 08-24-Add_build_trace_functions_for_MLE_eval_component branch from 4f77d82 to e67c1c5 Compare August 28, 2024 03:07
@andrewmilson andrewmilson force-pushed the 08-23-Remove_InteractionElements_and_LookupValues branch from c6bfa66 to 4e6c433 Compare August 28, 2024 03:18
@andrewmilson andrewmilson force-pushed the 08-24-Add_build_trace_functions_for_MLE_eval_component branch from e67c1c5 to ce9c26a Compare August 28, 2024 03:18
@andrewmilson andrewmilson force-pushed the 08-23-Remove_InteractionElements_and_LookupValues branch from 4e6c433 to bdbb4a3 Compare August 28, 2024 15:13
@andrewmilson andrewmilson force-pushed the 08-24-Add_build_trace_functions_for_MLE_eval_component branch from ce9c26a to 028665a Compare August 28, 2024 15:13
@andrewmilson andrewmilson force-pushed the 08-23-Remove_InteractionElements_and_LookupValues branch from bdbb4a3 to 5ac666f Compare August 29, 2024 14:38
@andrewmilson andrewmilson force-pushed the 08-24-Add_build_trace_functions_for_MLE_eval_component branch from 028665a to 7fd99fa Compare September 1, 2024 06:38
@andrewmilson andrewmilson force-pushed the 08-23-Remove_InteractionElements_and_LookupValues branch from 5ac666f to bd7595e Compare September 3, 2024 06:17
@andrewmilson andrewmilson force-pushed the 08-24-Add_build_trace_functions_for_MLE_eval_component branch from 7fd99fa to b58bab4 Compare September 3, 2024 06:18
@andrewmilson andrewmilson force-pushed the 08-23-Remove_InteractionElements_and_LookupValues branch from bd7595e to 8a565ba Compare September 9, 2024 06:13
Base automatically changed from 08-23-Remove_InteractionElements_and_LookupValues to dev September 9, 2024 06:58
@andrewmilson andrewmilson force-pushed the 08-24-Add_build_trace_functions_for_MLE_eval_component branch from b58bab4 to 1e9e342 Compare September 9, 2024 06:59
@andrewmilson andrewmilson merged commit 1f9187e into dev Sep 9, 2024
15 of 16 checks passed
@andrewmilson andrewmilson deleted the 08-24-Add_build_trace_functions_for_MLE_eval_component branch September 9, 2024 07:04
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