docs(m2): record comparison value semantics - #19
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 129335fded
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * mean difference; | ||
| * median difference; | ||
| * positive-pair count and rate; |
There was a problem hiding this comment.
For lower-is-better metrics, raw_difference and improvement_difference have opposite signs, but this result shape exposes only an unnamed mean and median while the positive-pair rate explicitly uses improvement differences. Implementations can therefore produce incompatible aggregate signs, such as a negative mean beside a 100% positive-pair rate. Specify which per-pair value is aggregated, or retain separately named raw and direction-aware aggregates.
Useful? React with 👍 / 👎.
| Runs are paired by seed. A comparison fails explicitly when: | ||
|
|
||
| * a selected run has no seed; | ||
| * the baseline and candidate seed sets do not match; | ||
| * a selected run lacks the requested summary metric or has a missing value; | ||
| * more than one run has the same variant and seed. |
There was a problem hiding this comment.
When callers select the same variant for baseline and candidate, every listed validation condition passes and each run can be paired with itself, silently yielding zero differences instead of a meaningful comparison. The comparison contract should explicitly require distinct baseline and candidate variants and report a selection error when they match.
Useful? React with 👍 / 👎.
| M2 will compare the requested metric from `summary_metrics` only. If the | ||
| metric is absent or its value is missing, the comparison fails explicitly. | ||
| There is no automatic fallback from summary metrics to metric history. |
There was a problem hiding this comment.
When a canonical Run contains NaN or infinity, the value is neither absent nor missing, so this policy permits it even though means, medians, differences, and positive-pair rates then become non-finite or misleading. Because Run.summary_metrics currently uses unconstrained floats, require selected values to be finite and cover those inputs in validation tests.
Useful? React with 👍 / 👎.
1 participant
Problem
What situation existed before this change? Why did it need to change?
Change
What did you actually do? Summarize the change, not the diff line-by-line.
Design Decisions
Any non-obvious choices made while implementing this — trade-offs, rejected
alternatives, or anything a future reader would ask "why did you do it this
way?" about.
Validation
pytestpassesruff check .passesmypy srcpassesRisks
What could this break? What wasn't tested? What assumptions might not hold
for other inputs or backends?
Related