Skip to content
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

Update FV scheme to use context chain instead of single context object #3962

Merged
merged 4 commits into from
Aug 10, 2020

Conversation

rav1kantsingh
Copy link
Member

@rav1kantsingh rav1kantsingh commented Aug 8, 2020

Description

For Relinearization operation, we need to perform modulus switching which requires the context chain to have context object with different coefficient modulus value.

Affected Dependencies

None

How has this been tested?

All tests for components passing

Checklist

@rav1kantsingh rav1kantsingh added the Type: Improvement 📈 Minor improvements not introducing a new feature or requiring a major refactor label Aug 8, 2020
@rav1kantsingh rav1kantsingh added this to the BFV Tensor in Python milestone Aug 8, 2020
@rav1kantsingh rav1kantsingh requested a review from a team as a code owner August 8, 2020 12:43
@rav1kantsingh rav1kantsingh changed the title Update FV scheme to use context chain instead of one context object Update FV scheme to use context chain instead of single context object Aug 8, 2020
Comment on lines 66 to 70
Args:
param: An EncryptionParams object with polynomial modulus, coefficient modulus and
plain modulus set.
prev_context_id: Id of the previous ContextData object in context chain.
next_context_id: Id of the next ContextData object in context chain.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the Args be in the init method?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I will put the whole doc component inside init


else:
raise ValueError("key for encryption is not valid")

def _encrypt(self, message, is_asymmetric):
def _encrypt(self, message, param_id, is_asymmetric):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can update the doc here

Comment on lines -38 to -39
if not is_initialized:
self._secret_key = SecretKey(sample_poly_ternary(param))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why no more init checking?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_initialized is no more needed because if the context is not set up it will throw the error while validation in context class. So the secret key can be created using context(anytime with validated context object). And for other keys, I will check for the None value of secret key.

@@ -109,7 +109,7 @@ def sample_poly_uniform(param):
return result


def encrypt_asymmetric(context, public_key):
def encrypt_asymmetric(context, param_id, public_key):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can update the doc here



def encrypt_symmetric(context, secret_key):
def encrypt_symmetric(context, param_id, secret_key):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@codecov
Copy link

codecov bot commented Aug 9, 2020

Codecov Report

Merging #3962 into master will increase coverage by 0.03%.
The diff coverage is 98.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3962      +/-   ##
==========================================
+ Coverage   94.85%   94.88%   +0.03%     
==========================================
  Files         202      202              
  Lines       20735    20879     +144     
==========================================
+ Hits        19669    19812     +143     
- Misses       1066     1067       +1     
Impacted Files Coverage Δ
test/torch/tensors/test_fv.py 100.00% <ø> (ø)
syft/frameworks/torch/he/fv/evaluator.py 95.51% <94.44%> (+0.44%) ⬆️
syft/frameworks/torch/he/fv/ciphertext.py 100.00% <100.00%> (ø)
syft/frameworks/torch/he/fv/context.py 88.88% <100.00%> (+8.33%) ⬆️
syft/frameworks/torch/he/fv/decryptor.py 100.00% <100.00%> (ø)
syft/frameworks/torch/he/fv/encryption_params.py 87.50% <100.00%> (+9.72%) ⬆️
syft/frameworks/torch/he/fv/encryptor.py 96.66% <100.00%> (+1.42%) ⬆️
syft/frameworks/torch/he/fv/integer_encoder.py 90.90% <100.00%> (ø)
syft/frameworks/torch/he/fv/key_generator.py 91.66% <100.00%> (-0.34%) ⬇️
syft/frameworks/torch/he/fv/util/operations.py 94.57% <100.00%> (+0.08%) ⬆️
... and 8 more

Copy link
Member

@youben11 youben11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@youben11 youben11 merged commit 092717c into OpenMined:master Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Improvement 📈 Minor improvements not introducing a new feature or requiring a major refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants