add noise level #651

mayeul-zama · 2023-10-23T09:21:47Z

No description provided.

github-actions · 2023-10-23T09:22:01Z

@slab-ci cpu_fast_test

github-actions · 2023-10-23T09:24:01Z

@slab-ci cpu_fast_test

IceTDrinker

Missing tests

tfhe/src/shortint/server_key/mod.rs

tfhe/src/shortint/parameters/multi_bit.rs

tfhe/src/shortint/engine/server_side/mod.rs

github-actions · 2023-10-23T09:52:33Z

@slab-ci cpu_fast_test

tmontaigu · 2023-10-23T10:31:38Z

tfhe/src/shortint/ciphertext/mod.rs

@@ -83,6 +123,7 @@ impl Degree {
 pub struct Ciphertext {
    pub ct: LweCiphertextOwned<u64>,
    pub degree: Degree,
+    pub noise_level: NoiseLevel,


So as we know adding this field breaks deserialization

Do we plan to do something to mitigate that ?

with the 0.5.0 we will do the semver trick, we will most likely provide a way to load old ciphertext and set the noise to the max for that ciphertext or do a bootstrapping right away I would say

for 0.5.0 I would try to break as many things as possible

@mayeul-zama didn't we say we want the noise level to be private to be able to crash with a given feature if the noise level exceeds a valid noise level ?

I don't remember that
I don't see how it's needed to have this crash feature

it might make identifying the faulty operation harder though if you have a chain of adds, so... not sure I like it much

Yes, and if you too many linear operations without a PBS after, the assert won't be triggered

Yeah its less precise and uses the fact that in practive you are very limited in the number of leveled operation you can do

But making it private forces the use of a constructor instead of a normal struct construction which is less ergonomic IMO

yes but that's a very small price to pay, even though it crossed my mind

github-actions · 2023-10-23T13:31:22Z

@slab-ci cpu_fast_test

IceTDrinker · 2023-10-23T13:34:34Z

tfhe/src/shortint/server_key/tests/noise_level.rs

+#[test]
+fn test_noise_level_propagation() {


one function per operator being tested please, with corner cases (i.e. 0 and other values for scalars)

and when I say per operator, it includes, default, checked, smart and unchecked with assign versions to be sure we cover all of the paths

there should be a way to re use the CPU test executor from Thomas to reuse a test for all variants

except smart versions because of the mutable references potentially but it would automate most of the rest to check all paths

github-actions · 2023-10-24T14:44:47Z

@slab-ci cpu_fast_test

IceTDrinker

Some minor stuff on testing (naming) and some randomized tests

IceTDrinker · 2023-10-25T11:58:31Z

tfhe/src/shortint/server_key/tests/noise_level.rs

+    let test_fn = |f: &dyn Fn(&ServerKey, &Ciphertext) -> Ciphertext,
+                   g: &dyn Fn(NoiseLevel) -> NoiseLevel| {


dyn is required so the closure works for everything ?

also better naming for f and g please

Renamed op and predicate

tfhe/src/shortint/server_key/tests/noise_level.rs

IceTDrinker · 2023-10-25T12:06:12Z

tfhe/src/shortint/server_key/tests/noise_level.rs

+use crate::shortint::parameters::PARAM_MESSAGE_2_CARRY_2_KS_PBS;
+use crate::shortint::{Ciphertext, ServerKey};
+
+fn test_1_ct_noise_level_propagation(sk: &ServerKey, ct: &Ciphertext) {


test_unary_op_... ? it's a proposal but I think it's the notation we use in the C api IIRC, the end of the name is good 👍 , nitpick

tfhe/src/shortint/server_key/tests/noise_level.rs

IceTDrinker · 2023-10-25T12:11:58Z

tfhe/src/shortint/server_key/tests/noise_level.rs

+    );
+}
+
+fn test_2_ct_assign_noise_level_propagation(sk: &ServerKey, ct1: &Ciphertext, ct2: &Ciphertext) {


binary_op_assign

tfhe/src/shortint/server_key/tests/noise_level.rs

IceTDrinker · 2023-10-25T12:15:30Z

thanks for the base of the testing infrastructure, some improvements can be made I believe but we are nearly there

github-actions · 2023-10-25T14:30:53Z

@slab-ci cpu_fast_test

github-actions · 2023-10-25T14:39:47Z

@slab-ci cpu_fast_test

github-actions · 2023-10-25T14:43:04Z

@slab-ci cpu_fast_test

IceTDrinker

Minor details, next one should be good to merge

IceTDrinker · 2023-10-26T11:57:59Z

tfhe/src/shortint/ciphertext/mod.rs

+    pub fn set_noise_level(&mut self, noise_level: NoiseLevel) {
+        self.noise_level = noise_level
+    }


should we require a max_noise_level right away ? it would mean needing to put it on the ServerKey, or maybe wait a bit more, we will probably require to re optimize all parameters sets and rework parameter sets to have the norm2 in there

I plan to add it in a subsequent PR

ok ! follow up issue please :)

https://github.com/zama-ai/tfhe-rs-internal/issues/313

tfhe/src/shortint/server_key/tests/noise_level.rs

tfhe/src/shortint/ciphertext/mod.rs

tfhe/src/shortint/server_key/tests/noise_level.rs

github-actions · 2023-10-31T15:26:19Z

@slab-ci cpu_fast_test

tmontaigu · 2023-11-02T09:55:44Z

Changes looks good to me

github-actions · 2023-11-02T13:59:27Z

@slab-ci cpu_fast_test

github-actions · 2023-11-02T17:43:20Z

Pull Request has been approved 🎉
Launching full test suite...
@slab-ci cpu_test
@slab-ci cpu_integer_test
@slab-ci cpu_multi_bit_test
@slab-ci cpu_wasm_test
@slab-ci csprng_randomness_testing

IceTDrinker

Looks good thanks !

cla-bot bot added the cla-signed label Oct 23, 2023

mayeul-zama requested review from IceTDrinker and tmontaigu October 23, 2023 09:22

mayeul-zama force-pushed the mz/add_noise_level branch from 9fcdd3f to 5a5ae33 Compare October 23, 2023 09:23

IceTDrinker requested changes Oct 23, 2023

View reviewed changes

tfhe/src/shortint/server_key/mod.rs Show resolved Hide resolved

tfhe/src/shortint/parameters/multi_bit.rs Show resolved Hide resolved

tfhe/src/shortint/engine/server_side/mod.rs Outdated Show resolved Hide resolved

tfhe/src/shortint/engine/server_side/mod.rs Outdated Show resolved Hide resolved

mayeul-zama force-pushed the mz/add_noise_level branch from 5a5ae33 to 92cfa11 Compare October 23, 2023 09:52

tmontaigu reviewed Oct 23, 2023

View reviewed changes

mayeul-zama force-pushed the mz/add_noise_level branch from 92cfa11 to b074a5c Compare October 23, 2023 13:31

mayeul-zama requested review from IceTDrinker and tmontaigu October 23, 2023 13:31

IceTDrinker reviewed Oct 23, 2023

View reviewed changes

mayeul-zama force-pushed the mz/add_noise_level branch from b074a5c to 925bdb4 Compare October 24, 2023 14:44

mayeul-zama requested a review from IceTDrinker October 24, 2023 15:17

IceTDrinker reviewed Oct 25, 2023

View reviewed changes

mayeul-zama force-pushed the mz/add_noise_level branch from 925bdb4 to de2978b Compare October 25, 2023 14:30

mayeul-zama requested a review from IceTDrinker October 25, 2023 14:36

IceTDrinker reviewed Oct 26, 2023

View reviewed changes

tfhe/src/shortint/server_key/tests/noise_level.rs Outdated Show resolved Hide resolved

IceTDrinker reviewed Oct 26, 2023

View reviewed changes

tfhe/src/shortint/server_key/tests/noise_level.rs Show resolved Hide resolved

IceTDrinker reviewed Oct 26, 2023

View reviewed changes

tfhe/src/shortint/server_key/tests/noise_level.rs Show resolved Hide resolved

tmontaigu reviewed Oct 30, 2023

View reviewed changes

tfhe/src/shortint/ciphertext/mod.rs Outdated Show resolved Hide resolved

tfhe/src/shortint/server_key/tests/noise_level.rs Outdated Show resolved Hide resolved

mayeul-zama force-pushed the mz/add_noise_level branch from bd320da to 7df4a9d Compare October 31, 2023 15:26

mayeul-zama requested review from IceTDrinker and tmontaigu October 31, 2023 15:36

mayeul-zama added 2 commits November 2, 2023 14:58

feat(shortint): add noise level to ciphertexts

5118065

feat(shortint): update noise level in operations

494a71c

mayeul-zama force-pushed the mz/add_noise_level branch from 7df4a9d to 494a71c Compare November 2, 2023 13:59

tmontaigu approved these changes Nov 2, 2023

View reviewed changes

github-actions bot added the approved label Nov 2, 2023

IceTDrinker approved these changes Nov 6, 2023

View reviewed changes

mayeul-zama merged commit 6497fb9 into main Nov 6, 2023
24 checks passed

mayeul-zama deleted the mz/add_noise_level branch November 6, 2023 10:33

		let test_fn = \|f: &dyn Fn(&ServerKey, &Ciphertext) -> Ciphertext,
		g: &dyn Fn(NoiseLevel) -> NoiseLevel\| {

		#[test]
		fn test_noise_level_propagation() {

add noise level #651

add noise level #651

Conversation

mayeul-zama commented Oct 23, 2023

github-actions bot commented Oct 23, 2023

github-actions bot commented Oct 23, 2023

IceTDrinker left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 24, 2023

IceTDrinker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IceTDrinker commented Oct 25, 2023

github-actions bot commented Oct 25, 2023

github-actions bot commented Oct 25, 2023

github-actions bot commented Oct 25, 2023

IceTDrinker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 31, 2023

tmontaigu commented Nov 2, 2023

github-actions bot commented Nov 2, 2023

github-actions bot commented Nov 2, 2023

IceTDrinker left a comment

Choose a reason for hiding this comment