Add GaussianSampler
#21
Conversation
|
src/core_crypto/guassian_sampler.rs
Outdated
| /// In particular, `sampled_val` is sampled from a normal distribution with mean 0 and standard deviation `sigma`. | ||
| /// If `sampled_val` is within the bounds, it is rounded and returned. | ||
| pub fn sample(&mut self) -> i64 { | ||
| let discrete_normal = Normal::new(0.0, self.params.sigma).unwrap(); |
There was a problem hiding this comment.
is new as no op?
If not then we should avoid creating new for each sample?
src/core_crypto/guassian_sampler.rs
Outdated
| pub fn sample(&mut self) -> i64 { | ||
| let discrete_normal = Normal::new(0.0, self.params.sigma).unwrap(); | ||
| let sampled_val = discrete_normal.sample(&mut self.rng); | ||
| if sampled_val.abs() < self.params.bound as f64 { |
There was a problem hiding this comment.
Remove the check and the recursive call because it guarantees that sample is within
There was a problem hiding this comment.
But shouldn't it be the case? Or in other words, wouldn't it be a problem if the sample is not within the bounds?
There was a problem hiding this comment.
I don't think there will be any problem. Value will be
Moreover, we aren't shooting for tight bounds at present so bounding seems unnecessary.
src/core_crypto/guassian_sampler.rs
Outdated
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Why do we need tests here if rand_distr already has tests?
There was a problem hiding this comment.
Probably the second and third test can be removed. What makes this implementation different from the standard rand_distr is that it is Discrete and Bounded. So probably test 1 should be maintained, but I'm sure that this is the required behavior given your previous comment
Remove the check and the recursive call because it guarantees that sample is within 6σ with 100% probability, which leaks information and is insecure.
There was a problem hiding this comment.
Ah but the discretised value is dependent on sample returned by rand_distr API. We have no control over it, so why are we testing it ?
|
Since we don't add anything on top of rand_distr should we move this directly inside |
Yes I think so. Just wanted to check your reply to the previous comments before moving it. |
|
commit fixes random_fill in RandomGaussianDist. We hardcode mean to 0 and std_dev to 3.2. |
The PR contains a simple
GaussianSampler. This adopts an approach similar to the one implemented by lattigo.There's no integration with any other part of the codebase. Opened it as a draft PR to ask you a review on the current implementation and further guidance on how to better integrate it into the rest of the codebase.