docs(m2): record comparison value semantics - #20
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; |
There was a problem hiding this comment.
For lower-is-better metrics, raw_difference and improvement_difference have opposite signs, but the result shape requests only an unspecified “mean difference” and “median difference.” An implementation could therefore report negative aggregate values for an improving candidate while its positive-pair rate says the candidate improved, or aggregate the interpreted values while callers expect candidate-minus-baseline. Specify which difference is aggregated, or expose aggregates for both.
Useful? React with 👍 / 👎.
| | Summary semantics | Preserve the value as `source-reported summary`; do not call it best or final | A source may define its summary as latest, best, or another aggregate | | ||
| | Missing metric/value | Reject the comparison with an explicit error | Avoid changing the denominator or silently producing partial evidence | | ||
| | Missing seed | A run without a seed cannot participate in pairing and is reported explicitly | Pairing requires an unambiguous key | | ||
| | Missing pair | Reject the comparison and report the unmatched seeds | A paired comparison is invalid when the baseline and candidate seed sets differ | |
There was a problem hiding this comment.
When neither requested variant exists, such as after misspelling both CLI arguments, the two seed sets are both empty and therefore satisfy the stated equality rule. None of the listed validation failures applies, yet mean, median, and positive-pair rate are undefined for zero pairs and will either fail during aggregation or produce an invalid result. Require at least one selected pair and cover the empty-selection case explicitly.
Useful? React with 👍 / 👎.
| | Input boundary | Comparison accepts an `ExperimentGroup`, not raw JSON | Keeps source ingestion separate from analysis and allows all local formats to converge first | | ||
| | Value source | Use `Run.summary_metrics[metric]` only | The source-provided scalar is available for M0 and M1 local formats without inventing checkpoint semantics | | ||
| | Summary semantics | Preserve the value as `source-reported summary`; do not call it best or final | A source may define its summary as latest, best, or another aggregate | | ||
| | Missing metric/value | Reject the comparison with an explicit error | Avoid changing the denominator or silently producing partial evidence | |
There was a problem hiding this comment.
The current Run.summary_metrics field accepts ordinary Python floats without a finiteness constraint, so a canonical or programmatically constructed group can contain NaN or infinity even though this decision rejects only absent or missing values. Such values propagate into a non-finite mean/median, and NaN > 0 silently counts as a non-positive pair, corrupting the reported evidence. Define non-finite metric values as invalid and include them in the validation scenarios.
Useful? React with 👍 / 👎.
|
|
||
| | Topic | M2 decision | Reason | | ||
| |---|---|---| | ||
| | Input boundary | Comparison accepts an `ExperimentGroup`, not raw JSON | Keeps source ingestion separate from analysis and allows all local formats to converge first | |
There was a problem hiding this comment.
If callers supply the same variant for both roles, selection returns the same runs on each side, every run is paired with itself, and all differences are reported as zero. Since no validation rule rejects this input, a CLI mistake can appear to be a valid neutral comparison rather than an error. Require the baseline and candidate variant names to be distinct.
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