fix(tdigest): interpolate quantiles toward the nearer centroid and normalize the left tail - #250
Open
jaideeppyne wants to merge 1 commit into
Open
Conversation
…rmalize the left tail
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TDigestView::quantilepasses its two interpolation weights toweighted_averagein the wrong order.w1is the distance from the left edge of the bracket andw2the distance to the right edge, butw1is paired withcentroids[i], so as the requested rank rises inside a bracket the estimate slides toward the lower centroid. The reference MergingDigest pairsmean[i]with the right-edge distance.This is reachable from plain
update(). I sampled 1001 evenly spaced ranks on each of 2000 digests of 1000 uniform values at k=100: all 2000 produced at least one rank wherequantile(r)came out belowquantileof a smaller rank, worst drop 56.99 on a 0..1000 range. Mean absolute rank error over 200 digests of 10000 values, measured against the true empirical rank of the returned value, goes from 0.011874 to 0.000552 after the fix.Two more, both in the mirrored tail branches:
quantileadds where its left-tail mirror subtracts, so the right tail walks pastmax. On a digest with centroids(10,w=10) (50,w=10) (90,w=10), min 0, max 100,quantile(0.9)returns 105.rankomits the/ total_weightnormaliser that its right-tail mirror has. The same digest returnsrank(5.0) = 3.0andpmf([5.0]) = [3.0, -2.0].Both tail branches need a first or last centroid heavier than one. Compression never merges the extreme centroids, so digests built through
updateormergealways keep unit-weight tails and never select them. I checked that empirically rather than assuming it: 200 C++ digests of 20000 values each serialized with unit-weight tails every time, and noupdate- ormerge-built digest in my sweep hit either branch. They are reachable throughdeserialize, including the reference implementation format thatdeserialize_compataccepts, where heavier tails are normal.How I found them: I was diffing quantile family behaviour against the C++ core from the
datasketchesPython package. That does not work here because the C++ core has all three defects too, byte for byte.tdigest_double.deserializeof the image above gives rank 3.0, quantile 105.0 and pmf[3.0, -2.0], identical to Rust. So I used invariants instead as the oracle: quantiles non-decreasing in rank and inside[min, max], ranks inside[0, 1]and non-decreasing in value, PMF masses non-negative and summing to 1. Sweep was 3000 digests, a third fromupdate, a third from twenty merges, a third deserialized with random centroid weights, 501 ranks and 501 values each. Zero violations after the fix, and I confirmed the pre-fix code violates every one of those except the left-tail rank normaliser, which only the deserialized third can reach. The same three are open against Java as apache/datasketches-java#755.Please note this makes Rust disagree with the current C++, Java and Go ports on
quantilefor any digest with more than one centroid. I think correctness wins here since the current output is not even monotonic, but it is a real cross-language divergence until the other ports land the same change, so tell me if you would rather hold this.Tests: three exact-value cases pinning each branch, plus
property.rswith quickcheck properties for quantile monotonicity fromupdateand frommerge, and for the CDF/PMF invariants. Five of the six fail on the current source with the tests kept in place.prop_cdf_is_a_distributionpasses before and after because the left-tail rank branch is not reachable fromupdate; the exact-value test covers that one. Fullcargo test --workspaceis green, including the cpp/java/go serde snapshots, andcargo x lintpasses excepthawkeye, which will not install on the pinned 1.86 toolchain.Not covered: the dead interpolation block after the loop in
quantile. I instrumented it with a panic and it was never reached across the 3000-digest sweep, since the guarded tail branch and theweight > total - 1early return between them cover the whole range. I left it alone rather than change unreachable code.I used Claude Code to help investigate and write this.