fix: report standard medians for even benchmark samples - #28
Merged
Merged
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The reviewed implementation, documentation, and regression coverage have no unresolved blocking issues.
Review effort: Lite
Findings: None
What changed in this PR
Corrects benchmark medians for even-sized samples while preserving existing percentile behavior.
Changes:
- Adds overflow-safe averaging of the two middle observations.
- Documents median and percentile conventions.
- Adds regression coverage for edge cases and unchanged tail percentiles.
| File | Summary |
|---|---|
docs/BENCHMARK.md |
Documents statistical conventions and legacy report handling. |
cmd/autocar/bench.go |
Implements corrected overflow-safe medians. |
cmd/autocar/bench_median_test.go |
Adds median and percentile regression coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Summary
median_mbpsandmedian_duration_msfor even sample counts by averaging the two middle observations.Regression evidence
median([2,10]) = 10instead of 6, andmedian([1,2,8,100]) = 8instead of 5.make check, fullmake race, formatting and independent review passed.2f4b67dca7cf744d8f4293f457c8db72a645054f: CI 35811582485 (all 11 jobs including container integration and OCI), CodeQL 35811582479, and Linux netem 35811582480 completed successfully.Scope
Benchmark reporting and documentation only. No transport/controller defaults, wire protocol, dependency, release or tag change. Raw samples and existing experimental binaries/data were not rewritten. This is a statistics correction, not a throughput improvement claim.
Merge verification
eecd1d992c46124c3390c0c9ddd918f74d47e011on main; its tree matches the tested PR head (07a234e7b47d57942b73fffc2ac079d6495e36dc).