feat: add diagnostic summaries for HLL and CPC - #232
Conversation
| let flavor = match self.flavor() { | ||
| Flavor::Empty => "Empty", | ||
| Flavor::Sparse => "Sparse", | ||
| Flavor::Hybrid => "Hybrid", | ||
| Flavor::Pinned => "Pinned", | ||
| Flavor::Sliding => "Sliding", | ||
| }; |
There was a problem hiding this comment.
I think you can either just use the Debug impl for flavor here or define a pub const fn as_str for Flavor`.
There was a problem hiding this comment.
Updated this to use the existing Debug implementation for Flavor.
| writeln!( | ||
| f, | ||
| " lower bound : {}", | ||
| self.lower_bound(NumStdDev::One) |
There was a problem hiding this comment.
Why NumStdDev::One? Could you point out the referenced implementation?
There was a problem hiding this comment.
Good point. The Java HLL summary uses one standard deviation, but the Java CPC summary does not include bounds. I removed the CPC lower and upper bounds instead of introducing that choice here.
| " upper bound : {}", | ||
| self.upper_bound(NumStdDev::One) |
There was a problem hiding this comment.
Removed this together with the lower bound, since the Java CPC summary does not include either bound.
There was a problem hiding this comment.
You may use insta for snapshot testing.
However, I have more to consider here now. Let me comment on the issue.
There was a problem hiding this comment.
Updated the exact Display assertions to use insta inline snapshots. I kept the populated cases as focused assertions so we do not snapshot estimator values unnecessarily.
326405e to
1c3e45b
Compare
|
@tisonkun, could you take another look when you have time? I addressed the review comments in |
|
@zhangxinyao88 I left a comment at #212 (comment) that at least we may not implement this function as |
zhangxinyao88
changed the title
|
Based on Lee's clarification in #212, this is informal, human-readable information about the sketch's current state. I think we can simply improve the existing I've pushed 6b14e51 to take this approach, using
|
2 participants
Part of #212.
Adds
summary()to HLL and CPC sketches and unions for diagnostic output that must not be parsed. This replaces the proposedDisplayimplementations and keeps coverage for empty and populated sketches.Testing
Not run locally:
cargois not available in this environment.