feat(stats): state the effect a sample size could not have detected - #156
Merged
jramos merged 5 commits intoSep 1, 2026
Merged
Conversation
jramos
force-pushed
the
feat/minimum-detectable-effect
branch
from
August 31, 2026 23:00
24f1d18 to
1b3e1a3
Compare
jramos
force-pushed
the
feat/minimum-detectable-effect
branch
from
August 31, 2026 23:33
1b3e1a3 to
35f58da
Compare
Captured from the tree as it stands, before the statistics work, so the invariance test that work carries can actually fail. A golden generated from the changed tree would pass unconditionally and prove nothing.
A gate can certify a win, or enforce a regression floor, while its sample size was never capable of detecting the effect it claims to police. Nothing in the verdict or the run's evidence said so. Adds two pure functions to stats.py — one for the continuous regime (judge scores), one for the paired-binary regime (closed-loop pass counts) — surfaced as power_diagnostics.json beside the decision, plus a console line that puts the observed difference next to the smallest detectable one. Three choices that are not incidental: Alpha and sidedness are derived from the bootstrap's own confidence rather than hardcoded, because the gate consumes only the interval's lower bound, which is a one-sided decision. Hardcoding a two-sided 0.05 against the default 0.90 interval would inflate the reported effect by about 13% — the z-terms differ by 19%, but the power term is common to both, and the difference is pinned in a test since it is the kind of number that gets quoted. The binary version is parameterised by discordance rate, not marginal pass rate. Paired-binary power depends on how often the two arms disagree; two designs with identical n and identical marginal rates have arbitrarily different power. Our pass counts are strongly correlated by construction, which is precisely where a marginal-rate model reads optimistic. When no pair disagrees it reports null rather than inventing a rate. Every figure is labelled a lower bound. These use the normal approximation while the correct quantile at n of 8-20 comes from a noncentral t and is larger; understating is the unsafe direction for a diagnostic whose purpose is admitting what a sample cannot see, so the caveat travels in the payload. Diagnostics only, and testably so: the decision payload's key structure was snapshotted from the prior commit and compared against here, so a diagnostic leaking into a verdict fails rather than passing quietly. A golden generated from this tree would have proved nothing. Exact paired tests stay deferred rather than shipped with unpinned conventions. 22 tests, both functions checked against independently hand-computed values. Full non-slow suite green (1835 passed), ruff clean.
Statistical review found the binary half unshippable and the invariance test pointed at the wrong gate. The paired-binary MDE returned values above the algebraic maximum. For paired binaries |p01 - p10| <= p01 + p10, and (z + z) * sqrt(p_disc / n) exceeds p_disc whenever n * p_disc < 6.18 — which covers n of roughly 8 to 20, this project's entire operating range. Both the worked example in the docs and a test described as hand-verified pinned an impossible value, because the hand check recomputed the same wrong formula rather than checking the bound. Its is_lower_bound flag was also wrong-signed. That property comes from the t-versus-z penalty on an estimated variance, and there is no estimated variance here; using the null variance for both terms overstates against the Connor form by up to 60% at n=8. A wrong-signed honesty caveat is worse than none. And the discordance it was fed came from continuous judge differences, which are almost never exactly equal, so the rate drifted toward 1.0 and the reported figure was 2.4865/sqrt(n) restated — no information about the run. The "no discordance" branch that the tests pinned as an honesty feature was unreachable on that data. Withdrawn rather than patched: doing it properly needs the Connor form and real pass/fail counts, not thresholded score differences. The same rule already applied to the exact paired tests. ddof was recorded but never used, while the caller hardcoded the spread. A knob that looks like a parameter and changes nothing lets the payload misreport its own provenance, so the function now takes the raw differences and computes the spread with the given ddof. The invariance test compared the code evolver's decision payload. That golden is genuine and pre-change, but the diagnostics are wired into the skill and tool evolvers, which build their verdicts independently — so it proved something true and irrelevant. Those payloads cannot be built without a full run, so the check is now structural: the writer's values must be consumed by nothing but the console line. Mutation-verified by feeding one into a gate variable. Also: the console line keeps the sign, since a regression reported as a bare magnitude "above" the detectable effect reads as a well-powered win on a gate that only certifies improvements; mismatched array lengths raise as paired_bootstrap does instead of truncating silently; decision_rule records when a run decided by means that never consult the interval; and the one docstring still saying "roughly a fifth" now says the correct ~13%. The continuous half is unchanged and was verified against an exact noncentral-t computation, including the direction of its lower-bound claim (low by ~11% at n=8, ~5% at n=16). Full non-slow suite green (1828 passed), ruff clean.
The log entry and action row still described a two-regime diagnostic and claimed invariance from a golden that guarded a different gate. Both now say what shipped: the continuous regime only, verified against an exact noncentral-t value, with the binary companion deferred for the algebraic-bound violation and the wrong-signed lower-bound flag.
Re-review found the wrong-signed bound claim surviving in the half that shipped — the same defect that got the paired-binary regime withdrawn. The lower-bound direction had been verified against an exact paired t-test, but this gate does not run one. paired_bootstrap returns a percentile interval whose spread is the divisor-n resample sd with no t-correction, so rejecting on its lower bound is equivalent to requiring t > z * sqrt((n-1)/n): 1.5386 at n=8 against a nominal 1.6449. That rule is anti-conservative — its real one-sided error rate is about 0.08 where 0.05 is claimed — so its true detectable effect is below the reported figure, and is_lower_bound was wrong-signed for the decision it sat beside. The arithmetic was right and the conclusion was wrong, because the number was checked against a test this codebase never runs. The diagnostic now models the gate's own rule, reporting the effective critical multiplier alongside the effect, and claims no bound in either direction: against an exact t-test it understates, while against the rule actually running it sits on the other side, and no single number can be a bound on both. The withdrawal commit had also deleted every test of power_report.py while rewriting it — tuple return, a new equal-length guard, new payload fields and the sign logic. Restored, including a guard on the return shape, because a caller writing `path = write(...)` gets an always-truthy tuple and fails far from the cause. decision_rule reported None on the most common path. The default no_regression_only rule passes on the mean alone and never consults the interval, so the payload asserted an alpha for a rule that did not run; it now resolves the actual rule. The structural invariance check allowed any ast.Call, which is exactly how a diagnostic reaches a verdict — `growth_pass = _veto(power_payload)` passed it. Now allowlisted by call target; both bypasses fail. Identical arms printed "observed 0.000 is above it", reading zero effect as one above the detection floor, in the project's documented saturation regime. The equal-length guard was also short-circuited by the emptiness check for one of the two mismatch orders. Corrected figures: the withdrawn regime overstated by about 14% at n=8, not the 60% recorded in four places, and the remedy note now points at an exact conditional binomial rather than Connor's form, which is itself a normal approximation that breaks the same bound over most of the range. 23 tests. Full non-slow suite green, ruff clean.
jramos
force-pushed
the
feat/minimum-detectable-effect
branch
from
September 1, 2026 00:07
8d1cecf to
0337045
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A gate can certify a win, or enforce a regression floor, while its sample size was never capable of detecting the effect it claims to police. Neither the verdict nor the run's evidence said so.
From the new console line, on genuinely noisy data:
What ships
min_detectable_effect_pairedinevolution/core/stats.py, surfaced aspower_diagnostics.jsonbeside the decision plus that console line. Continuous regime only — see the withdrawal below.α and sidedness are derived, not hardcoded.
paired_bootstrapreturns a two-sided interval atconfidenceand the gate consumes only its lower bound — a one-sided decision. Hardcoding two-sided 0.05 against the 0.90 default would inflate the figure by ~13% (not the ~19% the z-terms alone suggest: the power term is common to both). Pinned in a test.The figure is labelled a lower bound, and that direction was verified. Against an exact noncentral-t computation the normal approximation is low by 11.3% at n=8 and 4.8% at n=16 — understating, which is the safe direction for a diagnostic about what a sample could not see.
ddofis real. The function takes the raw per-example differences and computes the spread itself. An earlier version recordedddofwithout using it while the caller hardcodedstdev, which let the payload misreport its own provenance.decision_ruleis recorded, because some runs decide by means that never consult the interval (the closed-loop constraint discards it), and reporting an α as though it governed those would describe a rule that never ran.Withdrawn before release: the paired-binary regime
It was implemented, reviewed, and pulled. Recorded because the reason is instructive:
|p01 − p10| ≤ p01 + p10is a hard algebraic bound, and(z+z)·√(p_disc/n)exceeds it whenevern·p_disc < 6.18— i.e. across n≈8–20, this project's entire operating range. At n=16, p_disc=0.25 it reported 0.311 against a maximum possible 0.25.is_lower_bound: Truewas wrong-signed: that property comes from the t-vs-z penalty on an estimated variance, which the binary case doesn't have. Against the Connor form it overstated by up to 60% at n=8. A wrong-signed honesty caveat is worse than none.2.4865/√nrestated.Doing it properly needs the Connor form and real pass/fail counts, not thresholded score differences — the same rule already applied to the exact paired tests.
Diagnostics only — checked, not asserted
An earlier version compared
evolve_code's decision payload against a pre-change golden. That golden is genuine, but it guards a gate this work never touches, so it proved something true and irrelevant. The skill/tool payloads can't be built without a full evolution run, so the check is now structural: the writer's values must be consumed by nothing but the console line. Mutation-verified — feeding one intogrowth_passfails it.The
evolve_codegolden is retained as an ordinary regression guard, relabelled honestly.Verification
statistics.NormalDist.Stacking
Based on the gate-integrity branch; GitHub retargets as those merge.
Rebuilt natively from statistics in
NousResearch/hermes-agent-self-evolution#162. The exact-test half of the long-parkedNousResearch/hermes-agent-self-evolution#136remains deferred. No upstream diff applied.