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

Enable 'rng.gen()'ing field types #591

Merged
merged 1 commit into from
May 1, 2024
Merged

Conversation

andrewmilson
Copy link
Contributor

@andrewmilson andrewmilson commented Apr 30, 2024

This change is Reviewable

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 16 files reviewed, 1 unresolved discussion (waiting on @alonh5)


crates/prover/src/core/fields/m31.rs line 129 at r1 (raw file):

}

impl Distribution<M31> for Standard {

I thought about only implementing these for #[cfg(test)] but it meant it wasn't implemented for benchmarks

@andrewmilson andrewmilson force-pushed the andrew/dev/field_rand branch 2 times, most recently from 2c35511 to a5e2491 Compare April 30, 2024 20:37
@codecov-commenter
Copy link

codecov-commenter commented Apr 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.34%. Comparing base (3b6a9f6) to head (dbb8ada).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #591      +/-   ##
==========================================
- Coverage   94.43%   94.34%   -0.09%     
==========================================
  Files          69       69              
  Lines        8755     8670      -85     
  Branches     8755     8670      -85     
==========================================
- Hits         8268     8180      -88     
- Misses        417      420       +3     
  Partials       70       70              

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

Copy link
Contributor

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 13 of 16 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @andrewmilson)


crates/prover/benches/field.rs line 12 at r2 (raw file):

pub fn m31_operations_bench(c: &mut criterion::Criterion) {
    let mut rng = SmallRng::seed_from_u64(0);

This will always generate the same values, why not use from_entropy()? It's in the default-features and I noticed you disabled them.
Same for rest of PR.

Code quote:

let mut rng = SmallRng::seed_from_u64(0);

crates/prover/src/core/backend/avx512/cm31.rs line 118 at r2 (raw file):

        let mut rng = SmallRng::seed_from_u64(0);
        let x = PackedCM31([
            PackedBaseField::from_array(std::array::from_fn(|_| M31::from(rng.gen::<u32>() % P))),

Same in rest of PR.

Suggestion:

rng.gen()

crates/prover/src/core/fields/m31.rs line 129 at r1 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

I thought about only implementing these for #[cfg(test)] but it meant it wasn't implemented for benchmarks

I think this is fine. Maybe add a comment it's not meant to be used for cryptography and therefore only for testing.

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: 9 of 17 files reviewed, 2 unresolved discussions (waiting on @alonh5)


crates/prover/benches/field.rs line 12 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

This will always generate the same values, why not use from_entropy()? It's in the default-features and I noticed you disabled them.
Same for rest of PR.

Generating the same values is a good thing as otherwise our tests are no longer reproducible which is bad.


crates/prover/src/core/backend/avx512/cm31.rs line 118 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Same in rest of PR.

Done.

Copy link
Contributor

@alonh5 alonh5 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 4 of 8 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andrewmilson)


crates/prover/benches/field.rs line 12 at r2 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Generating the same values is a good thing as otherwise our tests are no longer reproducible which is bad.

Okay, I agree. Also for benching?

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: all files reviewed, 1 unresolved discussion (waiting on @alonh5)


crates/prover/benches/field.rs line 12 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Okay, I agree. Also for benching?

Sure, would be bad if from_entropy values crashed a benchmark sporadically.

Less important but still worth noting that I believe from_entropy has issues when building for some targets i.e. WebAssembly.

Copy link
Contributor

@alonh5 alonh5 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @andrewmilson)

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.

None yet

3 participants