Repository navigation
Conversation
… rounded ones
Every paired significance block gains exact: {delta_mean, ci_lo, ci_hi,
p_adjusted}, the values before rounding. The display fields keep their
rounding (delta_mean 2 decimals, CI and p 4), so existing readers see the
same numbers; a consumer that publishes the effect or re-derives a
verdict reads exact. mean_ci and bootstrap_ci take ndigits=None for the
unrounded interval.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
There was a problem hiding this comment.
🟡 Changes recommended
The test uses too few pairs to produce non-null p-values, so it passes even when corrected p-value handling is a no-op and cannot detect regressions.
1 open issue
What changed in this PR
1 new finding.
This PR preserves unrounded paired statistics alongside existing display values, supporting precise downstream reporting such as the skill evidence export in #344.
Changes:
- Adds exact deltas, confidence intervals, and corrected p-values while preserving display rounding.
- Propagates exact family corrections through arm significance and model comparisons.
- Adds precision tests, a changelog fragment, and an updated file-size baseline.
| File | Description |
|---|---|
dokimi/tests/test_stats.py |
Updates the expected confidence-interval arguments. |
dokimi/tests/test_stats_exact.py |
Adds tests for exact statistics and corrected p-values. |
dokimi/scripts/growth_baseline.json |
Lowers the model comparison file-size baseline. |
dokimi/dokimi/report/model_compare.py |
Stores exact corrected p-values for model comparisons. |
dokimi/dokimi/eval/stats.py |
Preserves unrounded statistics and adds exact correction support. |
dokimi/changelog.d/20261101_100000_napetrov_exact-stats.md |
Documents the additive exact-statistics fields. |
Review effort: Balanced
| def test_exact_p_adjusted_is_the_family_correction_before_rounding(): | ||
| results = [stats.paired_test([1.0, 2.5, 3.1, 4.7], [0.0, 0.1, 0.0, 0.2], rounded=False), | ||
| stats.paired_test([0.3, -0.2, 0.9, 0.1], [0.0, 0.0, 0.0, 0.0], rounded=False)] | ||
| stats._apply_correction(results, correction="holm", label="arms") | ||
| exact = stats.adjust_pvalues([r["p_ttest"] for r in results], method="holm", rounded=False) | ||
|
|
||
| for res, p in zip(results, exact, strict=True): | ||
| assert res["exact"]["p_adjusted"] == p | ||
| assert res["p_adjusted"] == stats._round_p(p) |
There was a problem hiding this comment.
Fixed in d4e40cf. Each comparison now has seven pairs, so both p-values exist. The test asserts that exact.p_adjusted is non-None and equals adjust_pvalues before rounding, and that the first comparison's exact value differs from its display value.
Mutation check: dropping the set_exact_p_adjusted call in _apply_correction now fails this test.
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 7 selected item(s). Review details
|
| results = [stats.paired_test([1.0, 2.5, 3.1, 4.7], [0.0, 0.1, 0.0, 0.2], rounded=False), | ||
| stats.paired_test([0.3, -0.2, 0.9, 0.1], [0.0, 0.0, 0.0, 0.0], rounded=False)] |
There was a problem hiding this comment.
Must-fix: both comparisons contain only four pairs, below MIN_PAIRS_FOR_TEST = 6, so their p_ttest values and both adjusted-p fields are always None. This test still passes if the new set_exact_p_adjusted() call is removed from _apply_correction, so it does not verify the behavior it names. Use at least six non-degenerate pairs per comparison with SciPy available, assert the corrected p-values are non-None, and choose inputs whose exact correction differs from its rounded display value.
There was a problem hiding this comment.
Fixed in d4e40cf: seven non-degenerate pairs per comparison (above MIN_PAIRS_FOR_TEST), and the test asserts the corrected exact p-values are non-None and equal the unrounded adjust_pvalues value.
Dropping set_exact_p_adjusted from _apply_correction now fails it.
The model_compare path has its own case in tests/test_model_compare_correction.py: one testable and one untestable run.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #345 +/- ##
=======================================
Coverage 94.05% 94.05%
=======================================
Files 297 297
Lines 38018 38028 +10
Branches 6424 6426 +2
=======================================
+ Hits 35756 35768 +12
Misses 1452 1452
+ Partials 810 808 -2
🚀 New features to boost your workflow:
|
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 296e636. The stats change is correct: the display fields are unchanged and exact is filled on the arm and model-compare correction paths. One risk: the new corrected-p test cannot fail. One nit inline.
What I ran, on a Linux review host at 296e636:
make ci-local: 30 passed, 0 failed, 7 skipped; 11,972 tests passed; changed-line coverage 100%- A mutation run on the new tests: dropping the
set_exact_p_adjustedcall from_apply_correctionpassestests/test_stats_exact.py(2 passed). - A repro of
model_compare.significance_teston 7 pairs (see the nit inline).
Checked, no issues
- Display fields:
delta_mean,ci_loandci_hiare the sameround(x, 2)/round(x, 4)of the same values as before. mean_ci/bootstrap_cicallers:ndigitsdefaults to 4, sochecklist.py,cross_judge.py,repos/compare.py,grounding.pyandbinary_stats.pyare unaffected.- Families with an untestable member:
exact.p_adjustedisNonefor that member and the unrounded value for the rest. - Commit: signed off.
| results = [stats.paired_test([1.0, 2.5, 3.1, 4.7], [0.0, 0.1, 0.0, 0.2], rounded=False), | ||
| stats.paired_test([0.3, -0.2, 0.9, 0.1], [0.0, 0.0, 0.0, 0.0], rounded=False)] | ||
| stats._apply_correction(results, correction="holm", label="arms") | ||
| exact = stats.adjust_pvalues([r["p_ttest"] for r in results], method="holm", rounded=False) | ||
|
|
||
| for res, p in zip(results, exact, strict=True): | ||
| assert res["exact"]["p_adjusted"] == p | ||
| assert res["p_adjusted"] == stats._round_p(p) |
There was a problem hiding this comment.
This test cannot fail when the corrected p-value is lost. Both comparisons have four pairs, below MIN_PAIRS_FOR_TEST = 6, so p_ttest is None, adjust_pvalues returns Nones, and the loop asserts None == None.
Mutation check: removing set_exact_p_adjusted(results, method=correction) from _apply_correction (stats.py:647) still gives 2 passed. The model_compare._adjusted_significance call has no test either.
Suggestion: use at least six non-degenerate pairs per comparison, e.g. [1.0, 2.5, 3.1, 4.7, 5.2, 6.0, 7.1] against [0.0, 0.1, 0.0, 0.2, 0.3, 0.1, 0.0], assert res["exact"]["p_adjusted"] is not None and equals the unrounded adjust_pvalues value. Add one case through model_compare._adjusted_significance with a testable and an untestable run.
There was a problem hiding this comment.
Thanks, fixed in d4e40cf. The arms test now uses your first input for one comparison, with seven pairs each, and asserts exact.p_adjusted equals the unrounded adjust_pvalues value.
Dropping the call in _apply_correction fails it. The _adjusted_significance case (two testable runs and one untestable) is in tests/test_model_compare_correction.py; dropping its set_exact_p_adjusted call also fails.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to a0f49ed, which fix the nit from my review:
a0f49ed:dokimi/dokimi/eval/stats.py:532:exactis built withoutp_adjusted;set_exact_p_adjustedadds the key only when a correction ran.
make ci-local at a0f49ed: 30 passed, 0 failed, 7 skipped.
Not approving yet. Still open: the risk from my review.
The two tests asserted None == None: four pairs per comparison sit below MIN_PAIRS_FOR_TEST, so p_ttest and p_adjusted were always None, and removing set_exact_p_adjusted left them green. Use seven pairs per comparison, assert the corrected exact p-values exist and equal adjust_pvalues before rounding. The model_compare case goes through _adjusted_significance with a testable and an untestable run, in a new file: test_model_compare.py is at the 1000-line cap. Mutation check: dropping the set_exact_p_adjusted call in _apply_correction or in _adjusted_significance now fails the matching test. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Summary
Every paired significance block (
summary.per_arm[arm].significanceinarms.v1, and the model-compare per-run family) gainsexact: {delta_mean, ci_lo, ci_hi, p_adjusted}, the values before rounding. Display fields keep their rounding (delta_mean2 decimals, CI and p 4), so existing readers see the same numbers. A consumer that publishes the effect or re-derives a verdict (e.g. the skill evidence export, #344) readsexact.mean_ci/bootstrap_citakendigits=Nonefor the unrounded interval;stats.set_exact_p_adjustedfills the unrounded family correction.Evidence
tests/test_stats_exact.py: exact values match unrounded math; rounded fields equalround(exact);exact.p_adjustedequalsadjust_pvalues(rounded=False)through_apply_correction.paired_teston 1200 random inputs: display fields byte-identical.Merge danger
Low: additive key; schema allows extra properties.
model_compare.pyhas a 2-line reflow to stay under its growth baseline.🤖 Generated with Claude Code