-
-
Notifications
You must be signed in to change notification settings - Fork 360
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 Approximate counting in Java #898
Add Approximate counting in Java #898
Conversation
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 for your first batch of PRs.
This looks like overall clean code, but I have a few problems with it, the vast majority of which are typos.
It's pretty merge-able right now, so I'll let you correct the problems 😄
* It terminates the program on failure | ||
*/ | ||
static void testApproximateCount(int nTrails, int nItems, double a, double threshold) { | ||
double avg = DoubleStream.generate(() -> approximateCount(nItems, a)) | ||
.limit(nTrails) | ||
.average() | ||
.getAsDouble(); | ||
|
||
if (Math.abs((avg - nItems) / nItems) < threshold) { | ||
System.out.println("passed"); | ||
} | ||
} |
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.
Shouldn't it terminate the program on failure? You don't seem to check anywhere.
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.
I'm not quite sure. I don't think this function should terminate the program on failure.
As I understand this function, it should only print whether the calculation was successful or not. However, I can add the else part to the if expression to print "failed" if the test has failed.
contents/approximate_counting/code/java/ApproximateCounting.java
Outdated
Show resolved
Hide resolved
contents/approximate_counting/code/java/ApproximateCounting.java
Outdated
Show resolved
Hide resolved
contents/approximate_counting/code/java/ApproximateCounting.java
Outdated
Show resolved
Hide resolved
contents/approximate_counting/code/java/ApproximateCounting.java
Outdated
Show resolved
Hide resolved
contents/approximate_counting/code/java/ApproximateCounting.java
Outdated
Show resolved
Hide resolved
This seems to vary from implementation to implementation. Julia's implementation returns Edit: I have created an issue for this - #907 |
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.
Pending #907, this is ready to merge.
I won't update right now until we have that sorted and standardized, so we are not too confused about merging it immediately
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.
These changes should make the implementation's output comply with the standardized version.
contents/approximate_counting/code/java/ApproximateCounting.java
Outdated
Show resolved
Hide resolved
#913 is merged and I commited @stormofice 'ssuggestion, so I will merge this |
No description provided.