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

Added Hook method and property -RSTensor #194

Merged
merged 4 commits into from Jun 2, 2021
Merged

Added Hook method and property -RSTensor #194

merged 4 commits into from Jun 2, 2021

Conversation

rasswanth-s
Copy link
Contributor

@rasswanth-s rasswanth-s commented May 25, 2021

Description

Modified the hook method and property of ReplicatedSharedTensor. fixes #177

Affected Dependencies

List any dependencies that are required for this change.

How has this been tested?

Added test for hook methods and property.

Checklist

Copy link
Contributor

@kamathhrishi kamathhrishi left a comment

Choose a reason for hiding this comment

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

So the overall structure of ReplicatedSharedTensor is as follows.

MPCTensor->ReplicatedSharedTensor->List[Torch Tensors]

So that means RSTensor holds shares of torch tensors and not sharetensors. RSTensor and ShareTensors are two seperate types of sharetensors.

src/sympc/tensor/replicatedshare_tensor.py Outdated Show resolved Hide resolved
src/sympc/tensor/replicatedshare_tensor.py Outdated Show resolved Hide resolved
src/sympc/tensor/replicatedshare_tensor.py Outdated Show resolved Hide resolved
tests/sympc/tensor/replicatedshare_tensor_test.py Outdated Show resolved Hide resolved
@kamathhrishi kamathhrishi added the feature Add a new functionality label May 25, 2021
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.

We have at the moment:
MPCTensor holds ShareTensor holds Torch Tensors

For Falcon it might be
MPCTensor holds RSS Tensor hold Torch Tensor

@rasswanth-s
Copy link
Contributor Author

rasswanth-s commented May 25, 2021

We have at the moment:
MPCTensor holds ShareTensor holds Torch Tensors

For Falcon it might be
MPCTensor holds RSS Tensor hold Torch Tensor

The init method of RSS Tensor had List[ShareTensor], I assumed as List[ShareTensor] ,I would refactor it

@kamathhrishi kamathhrishi self-requested a review May 26, 2021 06:36
@kamathhrishi
Copy link
Contributor

Looks good to me. @gmuraru and @aanurraj Have a look at it.

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! @aanurraj could you have a look?

@aanurraj aanurraj merged commit 479e825 into OpenMined:main Jun 2, 2021
@rasswanth-s rasswanth-s deleted the RST_hook branch June 2, 2021 17:01
@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.

Replicated Shared Tensor (RST): Hook method and Property
4 participants