Skip to content

feat: benchmaxxing datasets stats manifest summary + sanity check (closes #113) - #143

Merged
sebasmos merged 1 commit into
mainfrom
feat/datasets-stats
Jul 22, 2026
Merged

feat: benchmaxxing datasets stats manifest summary + sanity check (closes #113)#143
sebasmos merged 1 commit into
mainfrom
feat/datasets-stats

Conversation

@felipeocampoos

Copy link
Copy Markdown
Collaborator

What

Adds benchmaxxing datasets stats <manifest>, a sanity-check summary for a manifest before it's used in an expensive run.

  • Loads the manifest via benchmaxxing.data.load_cases (which already enforces the hard invariants -- empty manifest, malformed rows, out-of-range answer_index -- raising before this command sees the data).
  • Prints: row count and per-modality counts, an MCQ options-per-case distribution, an optional on-disk image_ref check via --image-root, the label distribution, and how many rows carry meta.
  • benchmaxxing datasets (no subcommand) is unchanged -- it still lists adapters; list is the explicit form of the same default, so nothing that already depends on the bare datasets command breaks.
  • On a missing/malformed manifest, prints a clear error: ... to stderr and exits 1 instead of a stack trace.

A bug I caught testing this

My first pass computed the answer_index range check as not (0 <= (c.answer_index or -1) < len(c.options)). Since 0 is falsy in Python, a perfectly valid answer_index=0 got treated as "missing" and flagged as out of range. Fixed to c.answer_index if c.answer_index is not None else -1, and added test_datasets_stats_accepts_answer_index_zero as a regression test so it can't come back silently.

Testing

tests/test_cli.py:

  • MCQ manifest summary (row/modality counts, options distribution, label distribution)
  • image manifest with and without --image-root
  • missing-manifest and empty-manifest non-zero exit
  • the answer_index=0 regression above
  • datasets and datasets list produce identical output

Also manually verified against a synthetic MCQ manifest and a synthetic imaging manifest (with a real file + a missing one) outside the test suite.

Suite: 402 passed, 6 skipped, ruff check . clean. The one failure I see locally (test_checksum_is_stable_and_matches_hashlib) is pre-existing on main (Windows CRLF vs a literal-string hash fixture), unrelated to this change.

CONTRIBUTING.md updated with the new subcommand.

Closes #113.

Copilot AI review requested due to automatic review settings July 20, 2026 14:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@sebasmos

Copy link
Copy Markdown
Member

Heads up @felipeocampoos, this now conflicts with main after #145 and #148 landed (both touched cli.py's subcommand registration). A quick rebase onto main should clear it; the datasets stats command itself was clean and passing (495 tests, ruff clean) when I tested the merge locally. Re-request review after the rebase and I'll land it.

@sebasmos

Copy link
Copy Markdown
Member

Thanks @felipeocampoos, datasets stats is a genuinely useful addition and the list + stats split is the right shape. Nice touch that a bare benchmaxxing datasets still defaults to list, so existing callers and the smoke pipeline keep working, and moving statsmodels into the datasets extra reads fine.

One fix before it can land: the branch looks based on a main from before #145, so the version command here has lost its --verbose flag (git SHA + installed extras). Could you rebase onto current main so version --verbose survives the merge? After that it is good to go, and it will slot in next to #142 (MedMCQA) once that rebases too.

…loses #113)

Adds a `stats` subcommand under `datasets` that loads a manifest via
benchmaxxing.data.load_cases and prints row/modality counts, an MCQ
options-per-case distribution, an optional on-disk image_ref check
(--image-root), the label distribution, and how many rows carry meta.

`benchmaxxing datasets` (no subcommand) still lists adapters as before --
`list` is the default when no datasets subcommand is given, so the existing
CLI/behavior is unchanged.

Hard invariants (empty manifest, malformed row) are enforced by load_cases
itself; the command catches FileNotFoundError/ValueError from the loader,
prints to stderr, and exits 1 instead of a stack trace. Options[answer_index]
range is also checked directly (belt-and-suspenders on top of the loader's
own validation) -- caught a self-introduced 0-is-falsy bug here during
testing (`c.answer_index or -1` treated a valid answer_index=0 as missing),
fixed and pinned with a dedicated regression test.

Tests (tests/test_cli.py): MCQ summary, image resolution with/without
--image-root, missing/empty manifest exit codes, the answer_index=0
regression, and that `datasets` and `datasets list` produce identical output.

CONTRIBUTING.md updated with the new subcommand.

Suite: 402 passed, 6 skipped, ruff clean (the one pre-existing failure,
test_checksum_is_stable_and_matches_hashlib, is a Windows CRLF issue on
main, unrelated to this change).
@felipeocampoos

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main -- version --verbose (#145) survives the merge, cli.py's subcommand registration is unified (version --verbose + datasets list/stats coexist), and the conflict is resolved. 588 passed, 6 skipped, ruff clean. Ready for another look.

@sebasmos sebasmos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, thanks @felipeocampoos. Rebased cleanly onto current main (diff is purely additive: cli.py + tests/test_cli.py + a CONTRIBUTING.md note, no deletions), version --verbose survives, datasets list/datasets stats coexist and a bare datasets still defaults to list. Verified independently: 587 passed, 7 skipped, ruff clean. Approving and merging.

@sebasmos
sebasmos deleted the feat/datasets-stats branch July 24, 2026 00:13
sebasmos added a commit that referenced this pull request Aug 4, 2026
feat: `benchmaxxing datasets stats` manifest summary + sanity check (closes #113)
sebasmos added a commit that referenced this pull request Aug 4, 2026
…corrected (closes #131)

Rebased fresh off current main (the original branch predated #142/#143/#146/#148/#154/#157/
#161/#219 and would have deleted all of that merged work if landed as-is).

Addresses @Agastya191's review on #159:
- flash_lite_reference was hardcoded to scale_c's PRE-parser-fix numbers (0.33/0.51 at n=150,
  p<1e-4). Now reads dynamically from --scale-c-summary (scale_c's committed summary, PR #141)
  so it cannot drift out of sync with a future fix there. Regenerated: 0.729/0.847 at n=85,
  p=0.041.
- Also caught in the same pass: the docstring and README's own flash numbers were ALSO stale
  (60 hard cases, generic 0.10/anchored 0.083) versus the actually-committed, already-corrected
  summary (28 hard cases, generic 0.714/anchored 0.679). Rewrote both to match the real data
  and restated the finding precisely: both tiers conform substantially to a bare peer (~0.71-0.73),
  but only flash-lite's conformity climbs further under a case-anchored rationale (+0.12, p=0.041);
  flash's does not move. The anchoring lever is model-dependent, not conformity itself.

Verified end-to-end: keyless reproduction confirmed with the key unset (new_api_calls_this_run: 0,
n_hard_cases: 28, matches exactly), ruff clean, 625 tests pass on the rebased base, no hardcoded
personal paths, 0 em dashes.
sebasmos added a commit that referenced this pull request Aug 4, 2026
…g subgroup (closes #185, closes #214)

Pure re-analysis of the already-committed imaging cascade transcripts, zero API calls.

#185: quantifies claim 4 (contagion rides on case plausibility, not the cue's own solo potency).
Spearman of solo flip-above-noise vs cascade contagion across the four cues is -1.0 (n=4,
descriptive): the weakest solo cue (cable) has the strongest cascade, the opposite of what a
cue-potency account predicts. The sharper test, per-case cross-cue agreement across the shared
35 cases, shows corner-tag/watermark/laterality adopt on the EXACT same cases (phi=1.0,
Jaccard=1.0); Cochran's Q across all four cues is not significant (p=0.392). Both support a
case-driven, not cue-driven, account of contagion.

#214: per-finding contagion breakdown (exploratory, n=35, cells as small as 1). No finding is
categorically immune or uniquely susceptible at this sample size; Wilson intervals are wide and
overlapping. Deliberately does not force a paired test across findings, since different findings
involve different cases (not a repeated-measures design), which would be an invalid comparison.

Also fixed along the way: PR #166's branch had gone stale relative to current main (predated
#219/#142/#143 merged since it was opened); merged main into the same branch in place (zero
reviews yet, so no review thread to disrupt) and re-verified keyless reproduction. This PR is
stacked on top of #166 since it needs the imaging results that only exist there.

Verified: ruff clean, 625 tests pass, no hardcoded personal paths, 0 em dashes, purely additive
(no existing file modified).
sebasmos added a commit that referenced this pull request Aug 4, 2026
…loses #108)

Rebased fresh off current main (the original branch predated #142/#143/#146/#148/#154/#157/
#161/#219 and would have deleted all of that merged work if landed as-is, same stale-base
issue as #143/#150/#141/#159).

Addresses @Agastya191's review on #158:
- README/PR body had stale pre-fix numbers (accuracy 0.29/0.14, flash flip 0.76 vs 1.00 at
  p=9.8e-5) versus the already-corrected committed summary (accuracy 0.89/0.78, flash flip
  0.079 vs 0.455 at p=3.3e-3, flash-lite 0.090 vs 0.636 at p=4.4e-7). Rewrote to match.
- ever_flipped collapsed the three cues into a per-case boolean that doesn't match the stated
  headline flip rate (0.063/0.117). Now reports both per_record_flip_rate (the headline
  quantity) and ever_flipped_rate, clearly labeled and distinguished.
- Grading-standard mismatch: clean_correct was imported from the prior solo pipeline rather
  than re-verified. Now re-graded by exact match against ground truth, the same standard as
  options_only_correct. Re-graded values are identical to the imported ones (0.89/0.78), so no
  actual discrepancy, but the standard is now transparent and consistent.
- Cache race: _Cache.complete checked the store under lock but called the API outside it, so
  two threads could both miss the same key and both append a duplicate entry (confirmed: the
  committed cache had exactly one). Lock now spans the full miss-check-fetch-store sequence.
  Cache also pruned from 6054 shared-cache entries down to the 400 this script actually
  requests (100 cases x 2 models x 2 probes).

Verified end-to-end: keyless reproduction confirmed with the key unset (new_api_calls_this_run:
0, all numbers match exactly), ruff clean, 625 tests pass, no hardcoded personal paths, 0 em
dashes.
sebasmos added a commit that referenced this pull request Aug 4, 2026
Rebased fresh off current main (the original branch predated #142/#143/#146/#148/#154/#157/
#161/#219 and would have deleted all of that merged work if landed as-is, same stale-base
issue as #143/#150/#141/#159/#158).

Addresses @Agastya191's review on #155:
- README/PR body carried pre-fix numbers (shared 0.175/0.15/0.175/0.15/0.15, p=1.0) versus the
  already-corrected committed summary (0.275/0.3/0.275/0.325/0.325, gained 3 lost 1 p=0.625).
  Rewrote to match and restated the finding as no significant compounding at p=0.625.
- Real bug: the isolated holdout's prompt was byte-identical to the bare prompt every round
  (board text was built only from peer votes, which are never visible in isolated mode), so at
  temperature 0 it deterministically reproduced round 1's answer every round. Isolated adoption
  was 0 by construction, not by measurement. Fixed by reminding the isolated holdout of its own
  previous-round answer (visible to it in isolated mode; only peer turns are hidden), so the
  prompt genuinely differs round to round. The shared arm's board construction is UNCHANGED, so
  the shared numbers are identical to what was already reviewed; only the isolated arm is new.

Result: isolated adoption is now a genuine (if still near-floor) measurement: 0.0/0.025/0.0/
0.025/0.0, at most 1 of 40 cases per round, rather than a trivial constant 0.0.

Verified end-to-end: ran for real with the isolated fix (206 new calls), then re-verified keyless
reproduction with the key unset (new_api_calls_this_run: 0, exact match on both curves), ruff
clean, 629 tests pass, no hardcoded personal paths, 0 em dashes.
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.

Tooling: datasets stats manifest summary + sanity check

5 participants