Skip to content

feat(sleep): paired A/B evalkit with McNemar and bootstrap CIs - #242

Open
Bogdan (Dan) Baciu (bogdanbaciu21) wants to merge 4 commits into
microsoft:mainfrom
bogdanbaciu21:exc-002-paired-evalkit
Open

feat(sleep): paired A/B evalkit with McNemar and bootstrap CIs#242
Bogdan (Dan) Baciu (bogdanbaciu21) wants to merge 4 commits into
microsoft:mainfrom
bogdanbaciu21:exc-002-paired-evalkit

Conversation

@bogdanbaciu21

@bogdanbaciu21 Bogdan (Dan) Baciu (bogdanbaciu21) commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Related: #108

Summary

This adds a stdlib paired A/B evalkit for claims that condition B beats condition A on one fixed task manifest:

python -m skillopt_sleep.evalkit --manifest tasks.json --a condition-a.json --b condition-b.json

Binary single-observation outcomes receive a two-sided exact McNemar result and paired bootstrap confidence interval. Graded scores are bootstrap-only and require --allow-graded. The nightly gate is unchanged.

Review fixes

  • Replaced the overflowing McNemar calculation with a numerically stable exact binomial tail. Tests cover more than 1,000 discordant pairs and the configured upper bound.
  • Reject empty seeds, non-finite or out-of-range scores, invalid alpha/bootstrap values, duplicate JSON keys, and ambiguous CLI inputs with EvalkitError. JSON serialization is strict and never emits NaN/Infinity.
  • Treat tasks as the independent unit for repeated seeds: scores are averaged within task, the bootstrap resamples whole tasks, and McNemar is omitted. A duplication regression proves extra seeds within one task cannot manufacture significance.
  • Retain array-versus-itself A/A only as an identity smoke test; seeded empirical-null tests now exercise type-I error and bootstrap CI coverage.

Validation

Exact head: 7c97a9915d0f614a5c016fec20db29bd3af71f42
Six-job native validation run

Runner Focused tests Entire repository suite
Linux / Python 3.12.14 193 passed, 84 subtests 1,469 passed, 11 skipped, 353 subtests
macOS arm64 / Python 3.12.10 193 passed, 84 subtests 1,469 passed, 11 skipped, 353 subtests
Windows Server 2025 / Python 3.12.10 189 passed, 4 skipped, 84 subtests 1,424 passed, 56 skipped, 353 subtests

No tests failed or were deselected. Ruff, compileall, git diff --check, and strict MkDocs also pass.

Add a stdlib evalkit so Sleep comparisons share one instrument: one
fixed task manifest, McNemar on paired binary outcomes, percentile
bootstrap CIs on the success-rate delta, and multi-seed variance bands.
Cross-manifest id mismatches are refused. The nightly gate is unchanged.

Related: microsoft#108
@bogdanbaciu21

Copy link
Copy Markdown
Contributor Author

Working and running the test suite comprehensively across Linux and Mac and Windows. Will try to finish today.

@bogdanbaciu21

Copy link
Copy Markdown
Contributor Author

Sorry it took me so long to get the full test suite out.

@Yif-Yang

Copy link
Copy Markdown
Contributor

Thanks for adding the paired-evaluation workflow and for running the cross-platform suite. I merged this head with current main; the full suite passes, but the statistical path still has correctness blockers that the tests do not cover.

  1. exact_mcnemar_p() evaluates math.comb(n, k) * (0.5 ** n). exact_mcnemar_p(700, 700) currently raises OverflowError: int too large to convert to float. Please use a numerically stable exact binomial-tail implementation (recurrence/log space, or a well-tested binomial survival function).
  2. Invalid inputs can produce invalid or misleading reports: empty seed arrays are accepted and yield NaN rates/CIs; NaN/Infinity scores can reach non-standard JSON; and an out-of-range alpha can produce nonsensical significance. Please reject empty seeds, non-finite/out-of-range scores, invalid bootstrap counts, and any alpha outside (0, 1) with EvalkitError, and ensure serialized JSON never contains NaN/Infinity.
  3. For multi-seed data, McNemar currently treats every (task, seed) pair as an independent observation, while the bootstrap operates on per-task means. This mixes inference units and can inflate significance through pseudoreplication. Please make the task the independent/cluster unit (or use and document an equivalent cluster-aware method) consistently across the reported inference.

Please add regressions for more than 1,000 discordant pairs, empty seeds, NaN/Infinity and out-of-range scores, invalid alpha/bootstrap values, strict JSON serialization, and a multi-seed fixture showing that duplicating seeds within one task does not spuriously increase significance. The current A/A test compares an array with itself, so it should also be replaced by a calibration test capable of catching type-I-error regressions. The feature is useful, but these issues need to be fixed before merge.

@bogdanbaciu21

Bogdan (Dan) Baciu (bogdanbaciu21) commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Yifan Yang (@Yif-Yang) Thanks for the detailed review. I fixed each requested blocker at head 7c97a9915d0f614a5c016fec20db29bd3af71f42:

  1. exact_mcnemar_p() now uses a log-space seed and recurrence-based exact tail, avoiding the overflowing math.comb(...) * 0.5**n calculation. Regression coverage includes more than 1,000 discordant pairs.
  2. Empty seeds, NaN/Infinity and out-of-range scores, invalid alpha, and invalid bootstrap counts now raise EvalkitError. JSON input/output is strict and cannot emit NaN/Infinity.
  3. Repeated-seed inference now uses tasks as clusters: scores are averaged within task and the bootstrap resamples whole tasks. McNemar is omitted for multi-seed inputs, and duplication regressions confirm repeated seeds cannot manufacture significance.
  4. The array-versus-itself check is retained only as an identity smoke test. Seeded empirical-null tests now exercise type-I error and bootstrap CI coverage.

I reran the focused and entire repository suites against that exact head on native Linux, macOS, and Windows. The full suite passed with 1,469 tests on Linux, 1,469 on macOS, and 1,424 on Windows; no tests failed or were deselected.

Six-job validation receipt

Ready for re-review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants