-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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 support for negative numbers in reciprocal method #4048 #4065
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4065 +/- ##
=======================================
Coverage 94.77% 94.78%
=======================================
Files 205 205
Lines 21187 21195 +8
=======================================
+ Hits 20081 20089 +8
Misses 1106 1106
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! But I would like to request some changes.
@@ -468,7 +468,7 @@ def matmul(self, *args, **kwargs): | |||
__matmul__ = matmul | |||
mm = matmul | |||
|
|||
def reciprocal(self, method="NR", nr_iters=10): | |||
def reciprocal(self, method="division", nr_iters=10): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, NR
is much faster than division
. Is there a reason for changing this? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#4036 (comment)... I saw this and made a change. I think now keeping it as NR should not be a problem.
pos = sgn | ||
neg = sgn - 1 | ||
sgn = pos + neg | ||
self = sgn * self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part duplicates many parts with NR
. Can you remove this duplication? 😎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes will do that!!
Can you have a look again @marload ? I have added a modulus and signum function instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
@marload it showing that I still need a review from openmined/cryptography! So that means anyone from that team can review right? |
result = 2 * result - result * result * new_self | ||
return result * self.signum() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very well implemented. Just a few code-quality comments. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
Also check out this
* Support for negative numbers * negative numbers support * Negative numbers support for reciprocal * negative nums support for reciprocal method
Description
This PR fixes #4048
Added support for negative numbers when use different methods in Reciprocal.
Uses a signum function to achieve this
Suggestions are welcome!
Affected Dependencies
How has this been tested?
Checklist