Conversation
Marvin-Beckmann
left a comment
Marvin-Beckmann
left a comment
There was a problem hiding this comment.
I think the current implementation is not working as intended after the mod is changed, see the respective comment
|
|
||
| impl ModulusPolynomialRingZq { | ||
| /// Changes the modulus `q` of the given [`ModulusPolynomialRingZq`] to the new modulus `q` | ||
| /// and resets all NTT-related attributes. |
There was a problem hiding this comment.
nice, I was wondering whether you thought about this :)
| pub fn change_q(&mut self, modulus: impl Into<Modulus>) { | ||
| self.ntt_basis = Rc::new(None); | ||
| self.non_zero = Vec::new(); | ||
|
|
||
| let new_poly_zq = | ||
| PolyOverZq::from((self.get_representative_least_nonnegative_residue(), modulus)); | ||
| self.modulus = Rc::new(new_poly_zq); | ||
| } |
There was a problem hiding this comment.
this would cause problems I think.
The non-zero needs to be set fitting to the modulus, i.e., it captures the non-zero coefficients used for the reduction -> see the reduce call in PolynomialRingZq that uses it. By setting this to 0, I think that essentially the polynomial reduction would always assume that you are using a modulus polynomial of X^N, i.e., there is no proper reduction.
On that note: I just found, that I missed to describe the new attributes from the changed modulus reduction on the Modulus struct.
Please add a test for this case, i.e., do some higher degree modulo multiplication, where the context polynomial is also used for reduction for the tests in the Polynomial constructions
There was a problem hiding this comment.
I've addressed the issue with the non-zero vector.
I'd suggest that you address the additionally missing parts described after "On that note" in a separate PR. You have a better idea what's needed / about the edge cases here.
There was a problem hiding this comment.
FYI I changed the non_zero vector s.t. it contains all positions of non-zero coefficients in the ModulusPolynomialRingZq. Previously, the highest coefficient (always 1) wasn't part of the vector. The tests all still pass, which tells me that either a test reg. this was missing or it didn't change anything and we might be doing double work somewhere now. As the one who wrote this logic, could you please check which one we want / need and add tests accordingly?
2 participants
Description
This PR implements...
change_moduluschange_qfor all non-NTT Zq datatypes.
Testing
Checklist: