-
Notifications
You must be signed in to change notification settings - Fork 5
PSF with Perturbation Sampling #60
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
Marvin-Beckmann
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.
looks good, just some minor comments and stuff that needs fixing
| fn samp_d(&self) -> MatZ { | ||
| let m = &self.gp.n * &self.gp.k + &self.gp.m_bar; | ||
| MatZ::sample_d_common(&m, &self.gp.n, &self.s).unwrap() | ||
| } |
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.
The s here should be the s the same one coming from the preimage sampling. The preimage sampling however has a factor of s*r and not just s
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.
Good catch
src/primitive/psf/perturbation.rs
Outdated
| // Sample perturbation p <- D_{ZZ^m, r * √Σ_p} - not correct for now. √Σ_p := as √Σ_2 | ||
| let vec_p = | ||
| MatZ::sample_d_common_non_spherical(&self.gp.n, mat_sqrt_sigma_2, &self.r).unwrap(); |
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.
What exactly do you mean with not correct?
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.
This is an old comment - have removed that part / revised the comment
| let sigma_2: MatQ = normalization_factor | ||
| * self.r.pow(2).unwrap() | ||
| * (&mat_sigma_p | ||
| - MatQ::identity(mat_sigma_p.get_num_rows(), mat_sigma_p.get_num_columns())); | ||
|
|
||
| // Compute √Σ_2 | ||
| sigma_2.cholesky_decomposition_flint() |
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.
the factors of r and normalization can also be added after the cholesky decomposition is computed as a linear factor.
This should/could speedup the computation time/precision of the cholesky decomposition, right?
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.
Yes, I've tried this out. It might increase precision, but it has a negative impact on performance as the cholesky_decomposition_flint uses f64 and thus, compresses the MatQ to some extend. If we multiply with any f64 or Q afterwards, that compression factor is becoming worse again. There might be a sweet spot between both, but I've chosen performance over precision for now
src/primitive/psf/perturbation.rs
Outdated
| /// | ||
| /// let (a, td) = psf.trap_gen(); | ||
| /// | ||
| /// let cov_mat = psf.s.pow(2).unwrap() * &psf.r * MatQ::identity(a.get_num_columns(), a.get_num_columns()); |
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.
This should be r**2, right?
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.
No, this is just a different covariance matrix with a different s. I've addressed it more explicitly now
| /// | ||
| /// Parameters: | ||
| /// - `mat_r`: The trapdoor matrix `R` | ||
| /// - `mat_sigma`: The covariance matrix `Σ` to sample [`Self::samp_p`] with |
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.
you are referring to matrix as Sigma_2 in the description - maybe rename this to sigma_2 or sth similar so it is clear that they are the same
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.
I've left it this way. Σ and Σ_p and Σ_2 are different matrices.
Σ is the covariance matrix as described.
We need Σ to compute Σ_p as described in Algorithm 3, MP12.
Last but not least, we pass Σ_p into Algorithm 1, Pei10, to sample the perturbation, which requires Σ_2, which needs to be computed from Σ_p. At the end, we only need to know sqrt of Σ_2 to perform Algorithm 3, MP12 out of those 3. Thus, we precompute Σ_2 from the covariance matrix Σ and store it
src/primitive/psf/perturbation.rs
Outdated
| /// - `short_basis_gadget`: The short basis of the corresponding gadget-matrix - just required to speed up the algorithm | ||
| /// - `short_basis_gadget_gso`: The GSO of the short basis of the gadget-matrix - just required to speed up the algorithm | ||
| /// | ||
| /// Returns a discrete Gaussian sampled preimage of `u` under `G` with Gaussian parameter `r * √(b^2 + 1)`. |
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.
what is b? Not necessarily clear from the documentation
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.
Made it more explicit in the doc 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.
The name of this file might not be enough to tell what it is related to. Maybe gpv_with_perturbation or gpv_perturbation or sth?
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.
I've named it mp_perturbation now as it doesn't have anything to do with GPV anymore and it is already in the psf folder. So, people know what to expect.
| /// - if Σ_2 is not positive definite. | ||
| pub fn compute_sqrt_sigma_2(&self, mat_r: &MatZ, mat_sigma: &MatQ) -> MatQ { | ||
| // Normalization factor according to MP12, Section 2.3 | ||
| let normalization_factor = 1.0 / (2.0 * Q::PI); |
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.
this seems to be what was missing in my previous implementation attempt.
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.
Yes
Description
This PR implements...
compute_sto compute the short basis ofGtoshort_basis_gadgetand puts it into thegadget_classicalfileTesting
Checklist: