Repository navigation
fix(evals): the 95th percentile is one rank too high when the sample size is a multiple of 20 - #698
devloper961-maker wants to merge 1 commit into
Conversation
…size is a multiple of 20 _percentiles() indexed the p95 with int(n * 0.95), which equals ceil(0.95 * n) - 1 only when 0.95 * n is not whole. At every multiple of 20 the product is exact, so the rank came out one too high: for the waits 1..20 ms the report said p95 = 20 ms where nearest-rank -- what the docstring promises -- is the 19th smallest, 19 ms. The clamp on top of it only masked the overflow past the last element, not the off-by-one inside the range. The index is now math.ceil(len * 0.95) - 1, which is always inside [0, n - 1], so the min() clamp goes away with the bug. Fixes NandhaKishorM#680. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <[email protected]>
|
Triage for the red |
Bruce-Yii
left a comment
Bruce-Yii
left a comment
There was a problem hiding this comment.
Flagging an overlap: this and #682 are the same fix, and the two of you will conflict if both are merged.
Both compute exact nearest-rank and both are correct. I checked yours independently against a reference written from the definition (exact rational ceil(95n/100), so the expected value never depends on float behaviour), swept to N=2,000,000: ordered[math.ceil(len(ordered) * 0.95) - 1] gives 0 disagreements, and so does #682's (95 * n + 99) // 100. Yours drops the now-unnecessary min(len-1, …) clamp, which is fine — ceil(0.95n) <= n for every n >= 1, so the index is always valid and there's no new IndexError path. The two differ in style, not behaviour; the float form is a little more readable, the integer form doesn't depend on how 0.95 rounds.
The failure mode matters more than the style question. git merge-tree reports a content conflict in laya/evals.py but auto-merges tests/test_evals.py — so a naive resolution lands both test_p95_is_nearest_rank_when_the_sample_size_is_a_multiple_of_20 and #682's test_percentiles_use_nearest_rank, testing the same thing twice. I resolved to your side and ran the artifact: 0 mismatches to N=3000, 60 passed. Correct, just carrying a redundant test.
The thing that actually differs is the close keyword, and it's in your favour. Yours says Fixes #680; #682 has no close keyword, so #680 stays open if #682 lands. #680 is a real report of this bug, not a self-filed duplicate, and it is distinct from the already-merged #592 (which only introduced the _percentiles helper — the off-by-one was there in the helper from the start). So landing this one closes it; landing #682 does not.
Coverage-wise it's close to a tie rather than one being strictly better. You uniquely pin n=21→20, which specifically guards against over-correcting to always n-1, plus n=2 and n=1. #682 uniquely pins n=100→95, n=30→29 and n=50→48, and a median assertion. If you'd rather take #682's implementation, the clean move is to take its test cases into this PR so the broader sizes are covered, and keep the close keyword here.
One nit on the description: it's truncated mid-sentence at "When \ is a multiple of 20, " — worth reposting in full.
|
Merged #682 first for the nearest-rank p95 fix. Closing this as duplicate. Thank you! |
Fixes #680
The bug
_percentiles()inlaya/evals.pyindexed the p95 as:int(n * 0.95)is the nearest-rankceil(0.95 * n) - 1only when0.95 * nis not whole. At every multiple of 20 the product is exact, so the rank came out one too high. Reproduced onmain(0.3.21) before the fix:Concretely: for 20 request waits of 1..20 ms, the report claimed
latency_p95_ms = 20where the 19th smallest — the nearest-rank p95 the docstring promises — is 19.The
min(len - 1, ...)clamp only stopped the index running past the last element atn = 20; it did nothing about the off-by-one inside the range (and at anynwhere the product exceedsn - 1, the clamp additionally returned the maximum, not the p95).The fix
ceil(0.95 * n) - 1is always in[0, n - 1](n ≥ 1), so the clamp disappears along with the bug —mathjoins the existing stdlib imports. Applies to both call sites,latency_p95_msandcost_per_decision_p95_ms.Regression test
test_p95_is_nearest_rank_when_the_sample_size_is_a_multiple_of_20intests/test_evals.py, next to the other timing tests: n = 20 → 19.0 and n = 40 → 38.0 (the exact-boundary cases), n = 21 → 20.0 (rank unchanged away from the boundary), plus n = 1 and n = 2 ends.Verification
🤖 Generated with Codebuff
Co-Authored-By: Codebuff [email protected]