-
Notifications
You must be signed in to change notification settings - Fork 71
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
Generate sketches for java #392
Conversation
Pull Request Test Coverage Report for Build 6032795106
💛 - Coveralls |
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.
A few minor things noted. Nothing serious, so no need to re-review after fixing comments.
for (unsigned i = 1; i <= n; ++i) sketch.update(std::to_string(i)); | ||
REQUIRE(sketch.is_empty() == (n == 0)); | ||
if (n > 10) { | ||
// REQUIRE(sketch.get_maximum_error() > 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.
no max error here?
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.
forgot to uncomment after some experiment
TEST_CASE("varopt sketch long sampling", "[serialize_for_java]") { | ||
var_opt_sketch<long> sketch(1024); | ||
for (unsigned i = 0; i < 2000; ++i) sketch.update(i); | ||
// heavy items have negative weights to allow a simple predicate to filter |
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 was an error in the comment in the original test. Negative values, not weights.
|
||
namespace datasketches { | ||
|
||
TEST_CASE("var opt union doulbe sampling", "[serialize_for_java]") { |
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.
typo: double
// small k sketch, but sampling | ||
var_opt_sketch<double> sketch1(k_small); | ||
for (unsigned i = 0; i < n1; ++i) sketch1.update(i); | ||
// heavy items have negative weights to allow a simple predicate to filter |
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.
s/weights/values/
|
||
TEST_CASE("theta sketch generate non-empty no entries", "[serialize_for_java]") { | ||
auto sketch = update_theta_sketch::builder().set_p(0.01).build(); | ||
sketch.update(1); |
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.
We do this for AOD, too. It does depend on the hash function vs p=0.01. I don't think it's critical but it is conditioning a test on an an implicit assumption around the hash function.
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.
right, but because 1 always has the same hash, and the test passes, it will stay that way
No description provided.