-
Notifications
You must be signed in to change notification settings - Fork 289
Am/test/noise check compression #2985
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
Conversation
mayeul-zama
left a comment
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.
@mayeul-zama reviewed 14 of 15 files at r1, all commit messages.
Reviewable status: 14 of 15 files reviewed, 4 unresolved discussions
-- commits line 12 at r1:
empty
scripts/pfail_estimate.py line 0 at r1 (raw file):
What is this file needed for?
tfhe/src/shortint/server_key/tests/noise_distribution/br_dp_packingks_ms.rs line 166 at r1 (raw file):
sanity_check_encrypt_br_dp_packing_ks_ms( TEST_PARAM_MESSAGE_2_CARRY_2_KS_PBS_TUNIFORM_2M128, TEST_COMP_PARAM_MESSAGE_2_CARRY_2_KS_PBS_TUNIFORM_2M128,
We could make sanity_check_encrypt_br_dp_packing_ks_ms take a single metaparam struct here instead of 2 parameter sets which may not correspond to each other
Same for other similar test functions
tfhe/src/shortint/server_key/tests/noise_distribution/br_dp_packingks_ms.rs line 642 at r1 (raw file):
// - so that the effective encoding after the storage is the one we used to evaluate the // pfail // TODO: do something about this
Should this be fixed now?
IceTDrinker
left a comment
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.
@IceTDrinker reviewed 1 of 15 files at r1, all commit messages.
Reviewable status: 14 of 15 files reviewed, 4 unresolved discussions (waiting on @mayeul-zama)
Previously, mayeul-zama wrote…
empty
yep, I opened the issue and updated the commit, will repush with the other changes
scripts/pfail_estimate.py line at r1 (raw file):
Previously, mayeul-zama wrote…
What is this file needed for?
It's not needed but it's a useful python tool I regularly use for pfail questions, seemed useful to commit to have in the repo
tfhe/src/shortint/server_key/tests/noise_distribution/br_dp_packingks_ms.rs line 166 at r1 (raw file):
Previously, mayeul-zama wrote…
We could make
sanity_check_encrypt_br_dp_packing_ks_mstake a single metaparam struct here instead of 2 parameter sets which may not correspond to each otherSame for other similar test functions
fair, is it ok with you if it's done in a follow up PR where I would do it for the various noise tests instead of making this PR even bigger than it is ?
tfhe/src/shortint/server_key/tests/noise_distribution/br_dp_packingks_ms.rs line 642 at r1 (raw file):
Previously, mayeul-zama wrote…
Should this be fixed now?
I could add a test primitive to be able to update the message mod yes
mayeul-zama
left a comment
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: 14 of 15 files reviewed, 3 unresolved discussions (waiting on @IceTDrinker)
Previously, IceTDrinker wrote…
yep, I opened the issue and updated the commit, will repush with the other changes
Ok
tfhe/src/shortint/server_key/tests/noise_distribution/br_dp_packingks_ms.rs line 166 at r1 (raw file):
Previously, IceTDrinker wrote…
fair, is it ok with you if it's done in a follow up PR where I would do it for the various noise tests instead of making this PR even bigger than it is ?
Sure
tfhe/src/shortint/server_key/tests/noise_distribution/br_dp_packingks_ms.rs line 642 at r1 (raw file):
Previously, IceTDrinker wrote…
I could add a test primitive to be able to update the message mod yes
Ok
- if a value is computed it will be correct - if the value is not finite (NaN or infinity) we panic with a message to the user indicating what course of action they can take - ideally we would want to use a scientific crate written in rust, xsf-rust seemed promising but the dependency on clang + libclang is proving more annoying than not, given we would need a single function from xsf (and it's hard to translate all the required pieces) we keep a sort of status quo - statrs issue : statrs-dev/statrs#361
- makes it clearer from which parameters some noise simulation primitives are built from
- noise checks and pfail based on expected noise have been added - compatible with KS PBS and KS32 PBS
2e181bf to
8532dfa
Compare
mayeul-zama
left a comment
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.
@mayeul-zama reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 14 of 15 files reviewed, all discussions resolved
added a pfail estimate script in the scripts dir if people want to have a quick tool to check and see what some results might be when doing some noise measurements
added tests for the compression atomic pattern, had to update some structs to be more dynamic for noise simulation (like being able to handle ciphertext moduli on u64 or u128, notably for the packing ksk)
one change compared to other tests is the iteration count which are adapted here to generate the same number of keys as the classic PBS ones for the pfail tests, noise checks are still lower in overall keys being generated because some confidence interval estimation functions can return NaN for large amounts of samples
lifted restrictions on the confidence interval, if the computation does not return finite f64 values we panic with an indication of what should be done to the user
This change is