-
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
Added the extra lower bound functions and test #289
Conversation
Pull Request Test Coverage Report for Build 2635063281
💛 - Coveralls |
tuple/include/tuple_sketch_impl.hpp
Outdated
@@ -39,16 +39,30 @@ double tuple_sketch<S, A>::get_estimate() const { | |||
return get_num_retained() / get_theta(); | |||
} | |||
|
|||
template<typename S, typename A> | |||
double tuple_sketch<S, A>::get_lower_bound(uint8_t num_std_devs, uint32_t num_subset_entries) const { | |||
if (is_empty()) num_subset_entries = 0 ; // When the sketch is empty we don't keep information for >0 identifiers. |
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 don't think this line is needed. if sketch is empty, get_num_retained() == 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.
I was originally unsure but I think that it is necessary. Suppose the line is not included and the user calls the function on an empty sketch but erroneously includes a num_subset_entries = x
for an x > 0. In this case, the sketch is empty, so the function should return 0. However, because the sketch is empty and not in estimation mode, we move to line 46 and return num_subset_entries = x
rather than zero. Likewise for the corresponding upper bound 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.
num_subset_entries will be set to 0 by line 45 since min(x, 0) == 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.
yes, i agree now!
Added extra lower bound functionality for consistency with java api and likewise for upper bound.
Expanded unit testing framework to account for these changes as well.