-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Addition operation on FV scheme #3674
Conversation
Check out this pull request on Review Jupyter notebook visual diffs & provide feedback on notebooks. Powered by ReviewNB |
Codecov Report
@@ Coverage Diff @@
## master #3674 +/- ##
==========================================
+ Coverage 94.79% 94.80% +0.01%
==========================================
Files 184 185 +1
Lines 18227 18292 +65
==========================================
+ Hits 17278 17342 +64
- Misses 949 950 +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.
Good job so far, just some minor issues and some questions.
A Plaintext object with value equivalent to result of addition of two provided | ||
arguments. | ||
""" | ||
pt1, pt2 = copy.deepcopy(pt1), copy.deepcopy(pt2) |
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.
same question regarding deepcopy
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.
In the case of Plaintext, it is not required, I removed it. Thanks!
arguments. | ||
""" | ||
pt1, pt2 = copy.deepcopy(pt1), copy.deepcopy(pt2) | ||
encoder = IntegerEncoder(self.context) |
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.
Will IntergerEncoder be the only supported encoder?
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, IntegerEncoder converts integers to plaintext and vice-versa and I don't have any other encoder for that.
assert ( | ||
int1 + int2 | ||
== encoder.decode(decryptor.decrypt(evaluator._add_plain_cipher(op1, op2))) | ||
== encoder.decode(decryptor.decrypt(evaluator.add(op1, op2))) |
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.
Maybe
== encoder.decode(decryptor.decrypt(evaluator.add(op2, op1)))
too?
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.
LGTM!
ct1, ct2 = copy.deepcopy(ct1.data), copy.deepcopy(ct2.data) | ||
result = ct2 if len(ct2) > len(ct1) else ct1 | ||
|
||
for i in range(min(len(ct1), len(ct2))): |
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.
Q: does this mean that if we have two ciphertext c1=(c1[0], c1[1])
and c2=(c2[0], c2[1], c2[2])
, the result would be c=(c1[0] + c2[0], c1[1] + c2[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.
extra c2[2] will be already present in the result by the:
result = ct2 if len(ct2) > len(ct1) else ct1
""" | ||
encoder = IntegerEncoder(self.context) | ||
|
||
return encoder.encode(encoder.decode(pt1) + encoder.decode(pt2)) |
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.
why should we require decoding, add and re-encode, can't we add in the plaintext space?
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.
After @bcebere suggestion on this method I tried to do:
return PlainText(poly_add_mod(pt1.data, pt2.data, self.plain_modulus))
but it did not return correct result. Any suggestions
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.
LGTM overall, I just have some questions and remarks
Description
Closes #3656
Addition operation on FV scheme.
Implemented:
How has this been tested?
Added tests for every private method and simultaneously tested the public method
add
function.Checklist