tisonkun commented on code in PR #209:
URL: https://github.com/apache/datasketches-rust/pull/209#discussion_r3841211938
##########
datasketches/src/tdigest/sketch.rs:
##########
@@ -762,54 +795,53 @@ impl TDigestMut {
/// # Contract
///
/// * `buffer` must have at least one centroid.
- /// * `buffer` is generated from `self.buffer`, and thus:
- /// * No `NAN` values are present in `buffer`.
- /// * We should clear `self.buffer` after merging.
+ /// * `buffer` contains all existing centroids and values from
`self.buffer`.
+ /// * No `NAN` values are present in `buffer`.
+ /// * We should clear `self.buffer` after merging.
fn do_merge(&mut self, mut buffer: Vec<Centroid>, weight: u64) {
- buffer.extend(std::mem::take(&mut self.centroids));
buffer.sort_by(centroid_cmp);
if self.reverse_merge {
buffer.reverse();
}
self.centroids_weight += weight;
- let mut num_centroids = 0;
+ let mut num_centroids = 1;
let len = buffer.len();
- self.centroids.push(buffer[0]);
- num_centroids += 1;
+ let centroids_weight = self.centroids_weight as f64;
+ let normalizer = scale_function::normalizer((2 * self.k) as f64,
centroids_weight);
let mut current = 1;
Review Comment:
Good catch. The expression predated this PR, but hoisting it made the
overflow reachable even for a one-value digest. Commit 3e02376 converts k to
f64 before multiplication and adds a u16::MAX regression test.
##########
datasketches/src/tdigest/sketch.rs:
##########
@@ -762,54 +795,53 @@ impl TDigestMut {
/// # Contract
///
/// * `buffer` must have at least one centroid.
- /// * `buffer` is generated from `self.buffer`, and thus:
- /// * No `NAN` values are present in `buffer`.
- /// * We should clear `self.buffer` after merging.
+ /// * `buffer` contains all existing centroids and values from
`self.buffer`.
+ /// * No `NAN` values are present in `buffer`.
+ /// * We should clear `self.buffer` after merging.
Review Comment:
Good catch. Updated the contract in 3e02376 to cover the centroids supplied
by both compress() and merge(), document the weight invariant, and clarify
finiteness and buffer clearing.
--
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]
