Copilot commented on code in PR #246:
URL: https://github.com/apache/datasketches-rust/pull/246#discussion_r3886217978
##########
tests-integration/tests/theta_test/jaccard_similarity.rs:
##########
@@ -193,26 +193,56 @@ fn
test_distinct_non_empty_sketches_with_no_retained_entries_are_uncertain() {
.sampling_probability(1e-12)
.build()
.unwrap();
+ sketch_a.update("apple");
+ sketch_b.update("banana");
+
+ assert!(!sketch_a.is_empty());
+ assert!(!sketch_b.is_empty());
+ assert_eq!(sketch_a.num_retained(), 0);
+ assert_eq!(sketch_b.num_retained(), 0);
+ assert_eq!(sketch_a.theta64(), sketch_b.theta64());
+
+ let operator = ThetaJaccardSimilarity::default();
+ assert_jaccard_exact(operator.compute(&sketch_a, &sketch_b).unwrap(), 1.0);
+ assert!(operator.exactly_equal(&sketch_a, &sketch_b).unwrap());
+}
+
+#[test]
+fn
test_distinct_theta_non_empty_sketches_with_no_retained_entries_are_uncertain()
{
+ let mut sketch_a = ThetaSketchBuilder::default()
+ .sampling_probability(1e-12)
+ .build()
+ .unwrap();
let mut different_theta = ThetaSketchBuilder::default()
.sampling_probability(2e-12)
.build()
.unwrap();
sketch_a.update("apple");
- sketch_b.update("banana");
different_theta.update("orange");
- assert!(!sketch_a.is_empty());
- assert!(!sketch_b.is_empty());
assert_eq!(sketch_a.num_retained(), 0);
- assert_eq!(sketch_b.num_retained(), 0);
assert_eq!(different_theta.num_retained(), 0);
Review Comment:
`test_distinct_theta_non_empty_sketches_with_no_retained_entries_are_uncertain`
relies on the two sketches having different theta values (to avoid the
identical-set shortcut), but it never asserts that precondition. If
`starting_theta_from_sampling_probability` rounding (or a future change) makes
`1e-12` and `2e-12` map to the same `theta64`, this test would start exercising
the wrong branch (and potentially fail for the wrong reason). Consider
asserting both sketches are non-empty and that `theta64` differs to keep the
test aligned with its intent.
##########
tests-integration/tests/tuple_test/jaccard_similarity.rs:
##########
@@ -150,12 +150,49 @@ fn
test_distinct_non_empty_sketches_with_no_retained_entries_are_uncertain() {
assert!(!sketch_b.is_empty());
assert_eq!(sketch_a.num_retained(), 0);
assert_eq!(sketch_b.num_retained(), 0);
+ assert_eq!(sketch_a.theta64(), sketch_b.theta64());
let operator = TupleJaccardSimilarity::default();
- let jaccard = operator.compute(&sketch_a, &sketch_b).unwrap();
+ assert_jaccard_exact(operator.compute(&sketch_a, &sketch_b).unwrap(), 1.0);
+ assert!(operator.exactly_equal(&sketch_a, &sketch_b).unwrap());
+}
+
+#[test]
+fn
test_distinct_theta_non_empty_sketches_with_no_retained_entries_are_uncertain()
{
+ let mut sketch_a = default_tuple_sketch_builder()
+ .sampling_probability(1e-12)
+ .build()
+ .unwrap();
+ let mut different_theta = default_tuple_sketch_builder()
+ .sampling_probability(2e-12)
+ .build()
+ .unwrap();
+ sketch_a.update("apple", 1u64);
+ different_theta.update("orange", 1u64);
+
+ assert_eq!(sketch_a.num_retained(), 0);
+ assert_eq!(different_theta.num_retained(), 0);
+
Review Comment:
`test_distinct_theta_non_empty_sketches_with_no_retained_entries_are_uncertain`
is intended to cover the different-theta case, but it doesn’t assert that the
sketches are actually non-empty or that `theta64` differs. Adding these
assertions makes the test robust against future changes (or rounding) that
could collapse the two sampling probabilities to the same theta and
accidentally exercise the identical-set path.
--
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]
