fix(metrics): make clearance_p5_m reproducible - #15
Merged
Merged
Conversation
tdigest.TDigest.compress() re-inserts centroids in pyudorandom order from an unseeded global random, so the same values give a different p5 every time -- four distinct values out of five builds in one process, relative spread 2.7e-03. Any A/B on this repository is affected. Seeding alone is not enough: merged_percentile also depends on input order, because t-digest merging is not associative. Both call sites already walk in sorted() order and a test pins that. getstate/seed/setstate is not re-entrant; both call sites are on the main thread and the invariant is documented. The reported percentile changes once. Past values were not comparable to each other either, so nothing is lost. 8 tests, all mutation-verified.
go-sakayori
approved these changes
Aug 12, 2026
HansRobo
added a commit
that referenced
this pull request
Aug 27, 2026
tdigest.TDigest.compress() re-inserts centroids in pyudorandom order from an unseeded global random, so the same values give a different p5 every time -- four distinct values out of five builds in one process, relative spread 2.7e-03. Any A/B on this repository is affected. Seeding alone is not enough: merged_percentile also depends on input order, because t-digest merging is not associative. Both call sites already walk in sorted() order and a test pins that. getstate/seed/setstate is not re-entrant; both call sites are on the main thread and the invariant is documented. The reported percentile changes once. Past values were not comparable to each other either, so nothing is lost. 8 tests, all mutation-verified.
HansRobo
added a commit
that referenced
this pull request
Aug 27, 2026
tdigest.TDigest.compress() re-inserts centroids in pyudorandom order from an unseeded global random, so the same values give a different p5 every time -- four distinct values out of five builds in one process, relative spread 2.7e-03. Any A/B on this repository is affected. Seeding alone is not enough: merged_percentile also depends on input order, because t-digest merging is not associative. Both call sites already walk in sorted() order and a test pins that. getstate/seed/setstate is not re-entrant; both call sites are on the main thread and the invariant is documented. The reported percentile changes once. Past values were not comparable to each other either, so nothing is lost. 8 tests, all mutation-verified.
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
2 participants
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.
Original PR: tier4#339
Problem
clearance_p5_mdoes not reproduce between identical runs.tdigest.TDigest.compress()re-inserts centroids in
pyudorandomorder from an unseeded globalrandom. Building the samedigest from the same values five times in one process gives four distinct p5 values (relative
spread 2.7e-03).
Any A/B on this repository is affected: the field moves on its own.
Fix
Seed the RNG around digest construction.
Seeding alone is not sufficient —
merged_percentilealso depends on input order, becauset-digest merging is not associative. Both call sites already walk in
sorted()order; a testpins that.
getstate/seed/setstateis not re-entrant. Both call sites are on the main thread; theinvariant is documented.
Note
The reported percentile changes once. There is no loss of comparability: past values were not
comparable to each other either.
Tests
8 tests, all mutation-verified (removing the seeding, or the
sorted(), fails them).