Skip to content

Fix swapped interpolation weights and tail branches in TDigestDouble - #755

Open
jaideeppyne wants to merge 2 commits into
apache:mainfrom
jaideeppyne:tdigest-rank-left-tail
Open

Fix swapped interpolation weights and tail branches in TDigestDouble#755
jaideeppyne wants to merge 2 commits into
apache:mainfrom
jaideeppyne:tdigest-rank-left-tail

Conversation

@jaideeppyne

@jaideeppyne jaideeppyne commented Aug 30, 2026

Copy link
Copy Markdown

Three spots in TDigestDouble differ from the reference implementation (MergingDigest). The first affects ordinary use, the other two need heapify.

I originally opened this for the third one only, then found the first while checking whether that fix was partial.

1. getQuantile() passes the interpolation weights in the wrong order

Reference:

double z1 = index - weightSoFar - leftUnit;
double z2 = weightSoFar + dw - index - rightUnit;
return weightedAverage(mean[i], z2, mean[i + 1], z1);

We compute the same two quantities as w1/w2 and then call weightedAverage(centroidMeans_[i], w1, centroidMeans_[i + 1], w2). The weight on mean[i] should be the distance to the far end, not the near one. As the rank rises inside a bracket the estimate moves toward the lower centroid instead of the upper one.

That makes getQuantile() non-monotonic from a plain update() sequence. 1999 of 2000 random digests (k in {10, 20, 50, 100, 200}, n in [100, 50000], gaussian / uniform / lognormal / sequential) return some getQuantile(r2) < getQuantile(r1) with r2 > r1. Worst seen: rank 0.9995 gives 16799.3 after rank 0.9990 gave 36135.9.

It costs accuracy too. Rank error against exact quantiles, 29700 queries at k=100, n=20000:

          mean       max
before    0.011966   0.042300
after     0.000675   0.004250

2. getQuantile() right tail has the wrong sign

Reference is max - (...), we have maxValue_ + (...), so the branch returns quantiles above getMaxValue(). On min 0, centroids (10,100),(20,100),(30,100), max 40, rank 0.90 returns 45.92 where the reference gives 34.08.

3. getRank() left tail is not normalized

It omits the / centroidsWeight_ that both the mirror-image right tail below it and the reference have, so getRank() leaves its documented [0, 1] range. Same digest, getRank(9.0) returns 45.1 instead of 0.1503. getCDF([5, 20, 35]) returns [25.5, 0.5, 0.915, 1.0], and getPMF returns a negative mass of -25.0.

2 and 3 are not reachable from update() or merge(), because compress() leaves both tail centroids singletons. They are reachable from heapify(), which accepts a first or last centroid of weight greater than 1.

Three named tests, each verified failing before its own fix. Full suite is 39 failures before and after, all *CrossLanguageTest with empty cpp_generated_files fixtures, unrelated to this.

C++ has all three the same way, and I reproduced the non-monotonic quantiles through the Python binding on the C++ core, 200 of 200 trials, so it probably wants the same changes.

I used Claude Code on this. The oracles were tdunning's MergingDigest and monotonicity, which needs no second implementation.

The left tail branch returned the interpolated weight without dividing by
centroidsWeight_, unlike the mirror-image right tail branch and the reference
implementation. Reachable via heapify() when the first centroid has weight
greater than 1, making getRank() exceed its documented [0, 1] range.
The interpolation passed weightedAverage(mean[i], w1, mean[i+1], w2) where the
reference passes the weights in the opposite order, so the estimate moved toward
the lower centroid as the rank rose within a bracket. This made getQuantile()
non-monotonic for ordinary update() sequences and cost about an order of
magnitude of rank accuracy.

The right tail also added the interpolated offset to maxValue_ instead of
subtracting it, returning quantiles above getMaxValue().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant