feat: add Display 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
Part of #212.
Summary
DisplayforHllSketch,HllUnion,CpcSketch, andCpcUnionExample output:
Testing
cargo x checkcargo x testcargo +nightly clippy --tests --all-features --all-targets --workspace -- -D warningscargo +nightly fmt --all --checkcargo +nightly doc --package datasketches --all-features --no-deps