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

Avg & Maxpooling with SMPC #3399

Merged
merged 5 commits into from
Apr 26, 2020
Merged

Avg & Maxpooling with SMPC #3399

merged 5 commits into from
Apr 26, 2020

Conversation

abogaziah
Copy link
Member

@abogaziah abogaziah commented Apr 23, 2020

Description

resolves issue #2573
overloading torch.nn.F.max_pool2d and avg_pool2d in order to use it in architectures like ResNet

Type of change

  • Added/Modified tutorials
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

@abogaziah abogaziah requested a review from LaRiffle April 23, 2020 22:07
@codecov
Copy link

codecov bot commented Apr 23, 2020

Codecov Report

Merging #3399 into master will decrease coverage by 0.04%.
The diff coverage is 98.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3399      +/-   ##
==========================================
- Coverage   94.70%   94.65%   -0.05%     
==========================================
  Files         150      152       +2     
  Lines       15949    16109     +160     
==========================================
+ Hits        15104    15248     +144     
- Misses        845      861      +16     
Impacted Files Coverage Δ
syft/frameworks/torch/nn/functional.py 96.87% <96.87%> (ø)
...orks/torch/tensors/interpreters/additive_shared.py 93.34% <100.00%> (ø)
test/torch/nn/test_functional.py 100.00% <100.00%> (ø)
syft/generic/pointers/object_pointer.py 78.19% <0.00%> (-3.76%) ⬇️
syft/generic/object.py 90.00% <0.00%> (-1.25%) ⬇️
syft/execution/plan.py 91.53% <0.00%> (-1.10%) ⬇️
syft/execution/state.py 89.62% <0.00%> (-0.65%) ⬇️
...ft/frameworks/torch/tensors/interpreters/native.py 90.88% <0.00%> (-0.45%) ⬇️
syft/frameworks/torch/hook/hook.py 93.43% <0.00%> (-0.08%) ⬇️
test/execution/test_plan.py 100.00% <0.00%> (ø)
... and 7 more

Copy link
Member

@Syzygianinfern0 Syzygianinfern0 left a comment

Choose a reason for hiding this comment

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

Real neat implem!! 🎉🎉
I've just left a comment in a couple of places that will make it more robust 😊

syft/frameworks/torch/nn/__init__.py Outdated Show resolved Hide resolved
syft/frameworks/torch/nn/functional.py Outdated Show resolved Hide resolved
syft/frameworks/torch/nn/functional.py Outdated Show resolved Hide resolved
syft/frameworks/torch/nn/functional.py Show resolved Hide resolved
@AlanAboudib
Copy link
Contributor

@Syzygianinfern0 did you check for any potential GC leaks?

Copy link
Contributor

@LaRiffle LaRiffle left a comment

Choose a reason for hiding this comment

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

One tiny suggestion

syft/frameworks/torch/nn/functional.py Outdated Show resolved Hide resolved
@Syzygianinfern0
Copy link
Member

@Syzygianinfern0 did you check for any potential GC leaks?

Major leaks currently. But testing on codebase of #3403 they disappear 🥳... Related issues I guess 🤔

@abogaziah abogaziah merged commit bc2acbd into OpenMined:master Apr 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants