feat: support theta and tuple a-not-b#155
Conversation
e4cbc3f to
2902e5e
Compare
32d85e8 to
1d29301
Compare
|
#154 is merged now. I wonder if we'd have a |
8b9e68f to
373584c
Compare
373584c to
ff2bb8d
Compare
ff2bb8d to
b557095
Compare
There was a problem hiding this comment.
I have some ideas on factoring out this test utility support module. Will do once this PR gets merged.
There was a problem hiding this comment.
... to avoid too much #[cfg(feature = "...")] and #[allow(dead_code)].
There was a problem hiding this comment.
Based on the result, it looks like the same as set difference.
@ariesdevil What do you think?
There was a problem hiding this comment.
a_not_b is a set difference. However the name a_not_b is a mnemonic that tells you the direction of the difference.
In the math of boolean logic one would write
A \ B := A AND NOT B =
or
B \ A := B AND NOT A =
Where "\" is the difference operator.
Note that A \ B != B \ A !
Although C++ allows user defined operators most languages do not. To call the difference function, we need to indicate the direction. We chose to do it by the position of the arguments, and the name of the function is a mnemonic of the direction.
Sketch result a_not_b(Sketch A, Sketch B);
There was a problem hiding this comment.
@leerho Thanks for your information!
Yes the Rust impl also defines the API as:
let update_policy = DefaultUpdatePolicy::<u64>::default();
let mut a = TupleSketchBuilder::new(update_policy).build();
a.update("apple", 1);
a.update("banana", 1);
let mut b = TupleSketchBuilder::new(update_policy).build();
b.update("banana", 1);
let a_not_b = TupleAnotB::default();
let result = a_not_b.compute(&a, &b, true).unwrap();So I generally agree that a_not_b provides more information and avoid the confusion of op direction.
Even if we can add methods or operator overrides to ThetaSketch/TupleSketch, it is not symmetry to the union and intersection interface.
One minor comment: any typical reason to name it TupleAnotB rather than TupleANotB? Weird CamelCase at first glance.
tisonkun
left a comment
There was a problem hiding this comment.
Generally LGTM.
Anything keeps this PR a draft before it's ready for review and merge?
tisonkun
left a comment
There was a problem hiding this comment.
Minor comments can be handled later.
based on #152 #153 #154