feat: add theta sketch (part 2) #59
Conversation
datasketches/src/theta/sketch.rs
Outdated
| /// assert!(lower_bound <= estimate); | ||
| /// assert!(estimate <= upper_bound); | ||
| /// ``` | ||
| pub fn get_lower_bound(&self, num_std_devs: u32) -> Result<f64, Error> { |
There was a problem hiding this comment.
As mentioned in the other PR, we don't need the get_ prefix.
datasketches/src/theta/sketch.rs
Outdated
| /// assert!(lower_bound <= estimate); | ||
| /// assert!(estimate <= upper_bound); | ||
| /// ``` | ||
| pub fn get_upper_bound(&self, num_std_devs: u32) -> Result<f64, Error> { |
* add binomial_bounds to support calculate lower_bound&&upper_bound * add get_lower_bound&&get_upper_bound in ThetaSketch
ZENOTME
force-pushed
the
binomial_bounds
branch
from
cf1c7b5 to
c4e321a
Compare
|
Waiting on requested change. |
| pub fn lower_bound(&self, num_std_devs: u32) -> Result<f64, Error> { | ||
| if !self.is_estimation_mode() { | ||
| return Ok(self.num_retained() as f64); | ||
| } | ||
| binomial_bounds::lower_bound(self.num_retained() as u64, self.theta(), num_std_devs) | ||
| } |
There was a problem hiding this comment.
We already have a NumStdDev enum:
datasketches-rust/datasketches/src/hll/estimator.rs
Lines 327 to 341 in 9ade42d
You can reuse it by moving the struct to a shared module level and thus we would never error here.
Besides, we may consider where to place these shared structs, including the ResizeFactor. datasketches::{NumStdDev, ResizeFactor} is fair enough now, but perhaps pollute the top-level namespace too much.
| // Comment for this larger test like Java | ||
| // for ci in 1..=3 { | ||
| // let arr = run_test_aux(2000, ci, 1e-7); | ||
| // for j in 0..5 { | ||
| // let ratio = arr[j] / STD[i][j]; | ||
| // assert!( | ||
| // (ratio - 1.0).abs() < TOL, | ||
| // "ci={}, j={}: expected {}, got {}, ratio={}", | ||
| // ci, | ||
| // j, | ||
| // STD[i][j], | ||
| // arr[j], | ||
| // ratio | ||
| // ); | ||
| // } | ||
| // i += 1; | ||
| // } |
There was a problem hiding this comment.
How much time it costs? I'd prefer not to comment out tests.
There was a problem hiding this comment.
I can't seem to find the source for this commented out test in either C++ or Java . Could you give me a full link?
tisonkun
left a comment
tisonkun
left a comment
There was a problem hiding this comment.
The rest should be OK.
https://github.com/apache/datasketches-rust/pull/59/changes#r2660026906 is significant.
4 participants
cc @notfilippo @Xuanwo @PsiACE @tisonkun