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

Sigmoid aprox with tanh + fix sigmoid #3205

Merged
merged 2 commits into from
Mar 22, 2020
Merged

Conversation

gmuraru
Copy link
Member

@gmuraru gmuraru commented Mar 16, 2020

Fixes #3131
Also adds the approximation for sigmoid using the tanh.

@gmuraru gmuraru changed the title Remove inverse for matrix Reverse inverse case for matrix Mar 17, 2020
@gmuraru gmuraru changed the title Reverse inverse case for matrix Remove inverse case for matrix Mar 17, 2020
else:
result = (1 + (self * -1).exp()).inverse()
one = self * 0 + 1
result = one / (1 + (self * -1).exp())
Copy link
Contributor

Choose a reason for hiding this comment

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

So basically you're suggesting .inverse() doesn't work ? :(
(The benefit of .inverse is that it's supposed to be faster)

Copy link
Contributor

Choose a reason for hiding this comment

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

But yeah you're right it's failiing apparently sometimes, I was discussing of this two weeks ago on the call with André

@gmuraru gmuraru changed the title Remove inverse case for matrix [WIP] Remove inverse case for matrix Mar 18, 2020
@gmuraru gmuraru changed the title [WIP] Remove inverse case for matrix Replace inverse with reciprocal + tanh approx Mar 19, 2020
@gmuraru gmuraru force-pushed the gm-smpc-fix branch 3 times, most recently from 5d9f6c0 to 5b302ef Compare March 19, 2020 08:48
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.

small comments

@@ -560,6 +607,7 @@ def _tanh_chebyshev(tensor, maxval: int = 6, terms: int = 32):
where c_i is the ith Chebyshev series coefficient and P_i is ith polynomial.
The approximation is truncated to +/-1 outside [-maxval, maxval].
Args:
tensor (tensor): values where the tanh needs to be approximated
Copy link
Contributor

Choose a reason for hiding this comment

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

typo this should be here :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm..this is for _tanh_chebyshev (forgot to put it in the previous PR)

@@ -449,6 +449,7 @@ def test_torch_sigmoid_approx(workers):
alice, bob, james = workers["alice"], workers["bob"], workers["james"]

fix_prec_tolerance_by_method = {
"chebyshev": {3: 6 / 100, 4: 1 / 100, 5: 1 / 100},
Copy link
Contributor

Choose a reason for hiding this comment

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

To improve test speed could we remove the case precision=5 everywhere where it occurs? From what I see =4 is already compelling enough :)

Approximates the sigmoid function
Compute the sigmoid using the exp approximation
sigmoid(x) = 1 / (1 + exp(-x))
sigmoid(x) = (sigmoid(|x|) - 0.5) * sign(x) + 0.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Detail a bit the method for people which don't know about numerical stability etc

@@ -449,6 +449,10 @@ def matmul(self, *args, **kwargs):
__matmul__ = matmul
mm = matmul

def reciprocal(self):
ones = self * 0 + 1
return ones / self
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep this like this, until I come back to trying my approx inverse trick

@gmuraru gmuraru force-pushed the gm-smpc-fix branch 2 times, most recently from fccba78 to 11aaece Compare March 21, 2020 15:14
@gmuraru gmuraru changed the title Replace inverse with reciprocal + tanh approx Sigmoid aprox with tanh + fix sigmoid Mar 21, 2020
@gmuraru gmuraru force-pushed the gm-smpc-fix branch 3 times, most recently from 8e732fe to 5f0d47d Compare March 21, 2020 17:46
@codecov
Copy link

codecov bot commented Mar 22, 2020

Codecov Report

Merging #3205 into master will decrease coverage by 4.74%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3205      +/-   ##
==========================================
- Coverage   99.30%   94.55%   -4.75%     
==========================================
  Files           4      146     +142     
  Lines        1866    15899   +14033     
==========================================
+ Hits         1853    15034   +13181     
- Misses         13      865     +852     
Impacted Files Coverage Δ
syft/codes.py 100.00% <ø> (ø)
syft/common/util.py 95.00% <ø> (ø)
syft/dependency_check.py 87.50% <ø> (ø)
syft/exceptions.py 77.35% <ø> (ø)
syft/execution/action.py 100.00% <ø> (ø)
syft/execution/communication.py 97.82% <ø> (ø)
syft/execution/computation.py 94.44% <ø> (ø)
syft/execution/placeholder.py 92.18% <ø> (ø)
syft/execution/plan.py 89.88% <ø> (ø)
syft/execution/protocol.py 88.02% <ø> (ø)
... and 166 more

@LaRiffle LaRiffle merged commit 87c9580 into OpenMined:master Mar 22, 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.

Sigmoid in SMPC failing for some shapes
3 participants