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

Create MLE eval prover component #804

Merged
merged 1 commit into from
Oct 1, 2024

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

Attention: Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.05%. Comparing base (cdf66f3) to head (5b3001a).
Report is 1 commits behind head on dev.

Files with missing lines Patch % Lines
crates/prover/src/core/air/accumulation.rs 0.00% 1 Missing ⚠️
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              
Flag Coverage Δ
92.05% <95.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@andrewmilson andrewmilson force-pushed the 08-20-Create_MLE_eval_component branch from 3a5da0f to c1ac882 Compare August 24, 2024 23:59
@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-20-Create_MLE_eval_component branch from c1ac882 to 3fde6e5 Compare August 26, 2024 04:15
@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
@andrewmilson andrewmilson force-pushed the 08-20-Create_MLE_eval_component branch from 3fde6e5 to bb56a26 Compare August 26, 2024 04:18
@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
@andrewmilson andrewmilson force-pushed the 08-20-Create_MLE_eval_component branch 2 times, most recently from 110a7b8 to 6b4cd33 Compare August 26, 2024 17:47
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 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?

@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-20-Create_MLE_eval_component branch from 6b4cd33 to 7af83e4 Compare August 28, 2024 02:03
@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-20-Create_MLE_eval_component branch from 7af83e4 to 3e7fdfa Compare August 28, 2024 02:11
@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-20-Create_MLE_eval_component branch from 3e7fdfa to 4e0bed5 Compare August 28, 2024 02:19
@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-20-Create_MLE_eval_component branch from 4e0bed5 to 4953abd Compare August 28, 2024 03:07
@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-20-Create_MLE_eval_component branch from 4953abd to 283d17a Compare August 28, 2024 03:18
@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-20-Create_MLE_eval_component branch from 283d17a to 62443c5 Compare August 28, 2024 15:13
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 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.

@andrewmilson
Copy link
Contributor Author

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

Previously, andrewmilson (Andrew Milson) wrote…

Updated to use the offset but honestly might be slower due to the bad access patterns.

Actually I'm wrong never mind

@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-20-Create_MLE_eval_component branch 2 times, most recently from cb773df to d8ff784 Compare September 1, 2024 21:59
@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-24-Add_build_trace_functions_for_MLE_eval_component branch from b58bab4 to 1e9e342 Compare September 9, 2024 06:59
Base automatically changed from 08-24-Add_build_trace_functions_for_MLE_eval_component to dev September 9, 2024 07:04
Copy link
Collaborator

@shaharsamocha7 shaharsamocha7 left a 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().

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

Copy link
Collaborator

@shaharsamocha7 shaharsamocha7 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 2 files at r3, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)

@andrewmilson andrewmilson merged commit cd1648e into dev Oct 1, 2024
15 of 16 checks passed
@andrewmilson andrewmilson deleted the 08-20-Create_MLE_eval_component branch October 1, 2024 10:21
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.

4 participants