fix(tdigest): correct rank and quantile tails - #531
Merged
Merged
Conversation
The left tail of get_rank() was not divided by the total weight, and the right tail of get_quantile() was added onto the maximum. A last centroid of weight 2 makes that division 0/0, so return the maximum. Interpolation now weights each centroid by the distance to the other one.
leerho
approved these changes
Oct 1, 2026
Member
There was a problem hiding this comment.
[Claude]: This is small and self-contained, and its three changes match the Java implementation line for line and fixes some real bugs.
- get_rank() left tail. Values below the first centroid weren't divided by the total weight, so the rank could exceed 1. That's clearly a bug, and the code even had a // ? comment on that line.
- get_quantile() right tail. The tail was added onto the maximum instead of measured back from it, so results could exceed the stored max. It also now returns max when the last centroid has weight 2, instead of dividing by zero and returning NaN.
- Interpolation weights. They were swapped, which pulled estimates toward the farther centroid.
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.
The C++ t-digest still disagrees with the Java, Go, and Rust sketches on three quantile and rank cases those ports already fixed.
get_rank()did not divide the left tail by the total centroid weight, so a value below the first centroid could report a rank greater than 1. The right tail was already normalized.get_quantile()added the right-tail offset onto the maximum, so estimates could land above the stored max. The tail is measured back from the maximum. A last centroid of weight 2 makes that division 0/0, and the only rank that reaches it is the maximum itself, so return the maximum.The new tests follow the Java cases: quantiles over 10k updates stay monotonic and inside
[min, max], a byte-patched heavy first centroid keeps ranks inside[0, 1], a heavy last centroid never exceeds max, weight 2 returns max rather than NaN, and an asymmetric interior point returns 13 rather than 17.Tests:
tdigest_test(55 cases).