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

Falcon : Select Shares #273

Merged
merged 124 commits into from
Aug 7, 2021
Merged

Conversation

rasswanth-s
Copy link
Contributor

@rasswanth-s rasswanth-s commented Jul 10, 2021

Description

Implementation of select shares functionality of Falcon, which takes input two tensors and outputs a tensor based on the selector bit b. Partially solves #147

How has this been tested?

Added Tests for the same.

Checklist


tensor_type = get_type_from_ring(session.ring_size)
mask = (b + c).reconstruct(decode=False).type(tensor_type)

Copy link
Contributor

@kamathhrishi kamathhrishi Aug 4, 2021

Choose a reason for hiding this comment

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

Also, I think it would be good to still do since the mask is a publlc value.

if mask=1:
       d = (1-c)
elif mask=0:
       d = c
else:
      raise ValueError("..")

It's computationally lesser expensive since we are using public operation. Doing it mathematically involves private operations making it expensive. Select shares is used in several operations such it would be good to get as much compute efficiency as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, Hrishi, we have to do it different for our tensor setting, the paper method won't work, I initially implemented what you suggested, what we want really want is ,given mpctensor x and y , we want a mix of values(some from x and some from y) ,depending on the selector bit tensor.

raise ValueError(
f"Invalid {ring_size} for selector bit,must be of ring size 2"
)
if shape is None:
Copy link
Member

Choose a reason for hiding this comment

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

Q: Do we ever get here? Since every MPCTensor should have a shape

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought of a scenario, when we initialize MPCTensor, with shares (actual tensor values not pointers) ,I thought to sanity check for shape.In the long run , we would have to include a sanity checks in MPCTensor which automatically infers shape from the shares.

Copy link
Contributor Author

@rasswanth-s rasswanth-s Aug 5, 2021

Choose a reason for hiding this comment

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

Binary sharing we use it only internally only with pointers, I thought of scenario, where we might mix the ring_size and give input as shares values , as pointer shares has a sanity check for shape.

Copy link
Member

@gmuraru gmuraru left a comment

Choose a reason for hiding this comment

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

Check comments. Really great job!

requirements.txt Outdated
@@ -1,4 +1,4 @@
sycret
syft @ git+https://github.com/OpenMined/PySyft@dev#egg=syft&subdirectory=packages/syft
syft @ git+https://github.com/OpenMined/PySyft@bb093294e98aa2be81412318fe8314cf6205c86b#egg=syft&subdirectory=packages/syft
Copy link
Member

Choose a reason for hiding this comment

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

Q: Can't we put dev? (or dev i still not working)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is fixed , I will revert back 👍

Copy link
Member

@gmuraru gmuraru left a comment

Choose a reason for hiding this comment

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

LGTM!

@rasswanth-s rasswanth-s merged commit 92fdc17 into OpenMined:main Aug 7, 2021
@rasswanth-s rasswanth-s deleted the select_shares branch August 7, 2021 01:28
@rasswanth-s rasswanth-s self-assigned this Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Add a new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants