Repository navigation
Conversation
Equal values are now ordered by ascending weight, so the merge result no longer depends on the input order or on the random pivot choice.
|
Thanks @luciakalf, this is a clean fix with no new dependencies, and the tests are good. It fixes a real bug: Before we merge, please make two small changes in
Please also state the tie-break rule in the Javadoc: centroids are ordered by mean ascending, then weight ascending. Once released, this rule becomes part of the sketch's behavior. What this achieves: with these changes, the same sequence of updates and merges should produce byte-identical sketches on any modern JVM (Java 17+), where all floating-point arithmetic is strict and Compatibility: the serialization format is unchanged, and images written before this change still deserialize and merge correctly, in Java and in the other languages. I'll send a note to dev@ describing the tie-break rule so the other language implementations can adopt it if they choose. |
|
Background on the tie-break rule, its effect on other language implementations, and why byte-identical images across languages aren't achievable for t-digest: #780 |
|
Thanks for the review! I’ve pushed the changes: ScaleFunction.z() now uses StrictMath.log, the normalizer is computed once before the loop in merge(), and the class Javadoc states the tie-break rule (mean ascending, then weight ascending). Sort.stableSort documents the same rule. mvn test passes on JDK 25. |
Fixes #739
What
Sort.stableSort(used byTDigestDouble.merge) ordered centroids only by value. Centroids with equal values but different weights ended up in an order that depended on the input order and on the random pivot, so the same input could produce different merged sketches.The sort now orders by value and, for equal values, by ascending weight, as agreed in the issue. It is pure Java with no new dependencies.
Changes
Sort.java: the quicksort and insertion sort compare (value, weight) pairs. The partition is a standard three-way partition around the pivot pair. Pairs equal to the pivot are identical, so the previous pivot-index handling is no longer needed.SortTest.java: new tests for the example from the issue, independence from input order (shuffled input), agreement with a reference sort for sizes around the insertion-sort threshold (63/64/65), and sorting only the requested prefix.Note: when
reverseMerge_is set, the sorted arrays are reversed, so equal values are then processed in descending weight. This is still deterministic.Testing
mvn teston JDK 25: 2294 tests, 0 failures (includingTDigestCrossLanguageTestandTDigestDoubleSerializationTest).TDigestDoublefrom the same 200,000 values with many duplicates 8 times produced 8 different serialized sketches before the change and 1 after. Quantiles are essentially unchanged.Performance
Simple timing of
Sort.stableSort(median, not JMH):This change was developed with the help of Claude (Anthropic), as suggested in the issue.