-
Notifications
You must be signed in to change notification settings - Fork 1
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
variance in nada-numpy #75
Conversation
generated using `nada generate-test --test-name variance variance`
m = self.mean() | ||
|
||
# Initialise the sum to 0 | ||
sum = Integer(0) |
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.
Nada Numpy also supports SecretRational types. The function should also work for Rational types. You can probably look into the implementation of other functions to check for this.
def nada_main(): | ||
parties = na.parties(4) | ||
|
||
a = na.array([4], parties[0], "A", SecretInteger) |
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.
Nada arrays do not require re-initialization of each of the entries. na.array
already does the initialization of the SecretInteger
s within it.
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 check is correct but doesn't check for other behaviors of the function and other datatypes. You should add the check for other variance values (!= 0
) and for other datatypes (Rational
, SecretRational
).
@@ -57,6 +57,7 @@ | |||
"shuffle", | |||
"array_comparison", | |||
"private_inverse", | |||
"var", |
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 should match the name of the Python executable, thus variance
|
||
# compute (x_i - mean)^2 | ||
|
||
for v in self.inner: |
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.
You should profit from broadcasting here. The function as it is, may not be working as you expect.
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.
Thanks a lot for the contribution. The code looks promising. Try to improve on the required changes and we'll be happy to review it again.
Resolves issue 62.