tisonkun commented on code in PR #93:
URL: https://github.com/apache/datasketches-rust/pull/93#discussion_r2807281798
##########
datasketches/src/bloom/builder.rs:
##########
@@ -88,8 +94,8 @@ impl BloomFilterBuilder {
/// # Panics
///
/// Panics if any of:
- /// - `num_bits` < MIN_NUM_BITS (64) or `num_bits` > MAX_NUM_BITS (~32 GB)
- /// - `num_hashes` < 1 or `num_hashes` > 100
+ /// - `num_bits` < MIN_NUM_BITS or `num_bits` > MAX_NUM_BITS
+ /// - `num_hashes` < MIN_NUM_HASHES or `num_hashes` > MIN_NUM_HASHES
Review Comment:
Constants are not included in the final public documents. We'd better to
speak out the exact value here.
##########
datasketches/src/bloom/builder.rs:
##########
@@ -108,8 +114,16 @@ impl BloomFilterBuilder {
"num_bits must not exceed {}",
MAX_NUM_BITS
);
- assert!(num_hashes >= 1, "num_hashes must be at least 1");
- assert!(num_hashes <= 100, "num_hashes must not exceed 100");
+ assert!(
+ num_hashes >= MIN_NUM_HASHES,
+ "num_hashes must be at least {}",
+ MIN_NUM_HASHES
+ );
+ assert!(
+ num_hashes <= MAX_NUM_HASHES,
+ "num_hashes must not exceed {}",
+ MAX_NUM_HASHES
+ );
Review Comment:
nit: `(MIN_NUM_HASHES..=MAX_NUM_HASHES).contains(&num_hashes)` and ditto
above for NUM_BITS.
But I'm OK to keep the current assertions. It's up to you.
##########
datasketches/src/bloom/builder.rs:
##########
@@ -215,7 +223,9 @@ impl BloomFilterBuilder {
/// assert_eq!(hashes, 7); // -log2(0.01) ≈ 6.64
/// ```
pub fn suggest_num_hashes_from_fpp(fpp: f64) -> u16 {
Review Comment:
This now follows other impls. But I wonder what if `k` is `NaN` or `Inf`
(e.g., `(-1.0f64).log2()`, `(0.0f64).log2()`, `f64::INFINITY.log2()`).
This can be a follow-up issue for coordinating among impls.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
