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

Add BatchNorm to AdditiveSharingTensor #3889

Merged
merged 3 commits into from
Jul 23, 2020
Merged

Add BatchNorm to AdditiveSharingTensor #3889

merged 3 commits into from
Jul 23, 2020

Conversation

LaRiffle
Copy link
Contributor

@LaRiffle LaRiffle commented Jul 22, 2020

Description

Add Batchnorm and tests
Fix a bug in .var() for evaluation on a specific dim

NOTE This implem will be improved with the current work on FALCON

Checklist

@LaRiffle LaRiffle added Type: New Feature ➕ Introduction of a completely new addition to the codebase Priority: 3 - Medium 😒 Should be fixed soon, but there may be other pressing matters that come first Severity: 3 - Medium 😒 Does not cause a failure, impair usability, or interfere with the system labels Jul 22, 2020
@codecov
Copy link

codecov bot commented Jul 22, 2020

Codecov Report

Merging #3889 into master will increase coverage by 0.00%.
The diff coverage is 95.83%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3889   +/-   ##
=======================================
  Coverage   94.98%   94.99%           
=======================================
  Files         186      186           
  Lines       19075    19122   +47     
=======================================
+ Hits        18119    18164   +45     
- Misses        956      958    +2     
Impacted Files Coverage Δ
...frameworks/torch/tensors/interpreters/precision.py 97.06% <50.00%> (-0.48%) ⬇️
syft/frameworks/torch/nn/functional.py 89.83% <100.00%> (+1.20%) ⬆️
test/torch/tensors/test_additive_shared.py 100.00% <100.00%> (ø)

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! Just left a small suggestion

Comment on lines +70 to +76
for i in range(80):
if x is not None:
y = C + 1 - var * (x * x)
x = y * x / C
else:
y = C + 1 - var
x = y / C
Copy link
Member

Choose a reason for hiding this comment

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

Why not executing the else outside, and removing condition in the for loop, since it's 1 vs 79 times.

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 agree, however we have 80 for bad reason, in the near future we'll have 3 to 5 iterations :)
I'm waiting for the FALCON team to do this :D

@LaRiffle LaRiffle merged commit 75d3d11 into master Jul 23, 2020
@delete-merged-branch delete-merged-branch bot deleted the ryffel/batchnorm branch July 23, 2020 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 3 - Medium 😒 Should be fixed soon, but there may be other pressing matters that come first Severity: 3 - Medium 😒 Does not cause a failure, impair usability, or interfere with the system Type: New Feature ➕ Introduction of a completely new addition to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants