Skip to content

fix(metrics): make clearance_p5_m reproducible - #339

Closed
HansRobo wants to merge 1 commit into
tier4-mainfrom
fix/tdigest-p5-reproducibility
Closed

HansRobo wants to merge 1 commit into
tier4-mainfrom
fix/tdigest-p5-reproducibility

Conversation

@HansRobo

@HansRobo HansRobo commented Jul 30, 2026

Copy link
Copy Markdown
Member

Problem

clearance_p5_m does not reproduce between identical runs. tdigest.TDigest.compress()
re-inserts centroids in pyudorandom order from an unseeded global random. Building the same
digest 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_percentile also depends on input order, because
t-digest merging is not associative. Both call sites already walk in sorted() order; a test
pins that.

getstate/seed/setstate is not re-entrant. Both call sites are on the main thread; the
invariant 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).

@HansRobo
HansRobo force-pushed the fix/tdigest-p5-reproducibility branch from d10cc13 to 8f92b67 Compare July 30, 2026 17:22
@HansRobo HansRobo changed the title fix: clearance_p5_m を再現可能にする(t-digest の RNG を固定) fix(metrics): make clearance_p5_m reproducible Jul 31, 2026
@HansRobo
HansRobo marked this pull request as draft July 31, 2026 00:08
@HansRobo
HansRobo force-pushed the fix/tdigest-p5-reproducibility branch 3 times, most recently from c5d08a8 to 810d0e0 Compare July 31, 2026 03:23
@HansRobo
HansRobo marked this pull request as ready for review July 31, 2026 03:23
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.
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.

2 participants