gate: per-cell budget for the mis-budgeted GMM row; exclude the confirmed adaptive-alloc regression from the probe - #59
Conversation
…ive-alloc regression
BUDGET. GMM mix_d6_n3_s303 sits on the n_eff=100 floor at the matrix budget, so
as a merge-BLOCKING row it is close to a coin flip. Measured over 8 fresh seeds:
budget n_eff min/med/max clears 100 median |bias|
x1 59 / 105 / 159 5/8 0.014
x2 105 / 169 / 214 8/8 0.010
x4 209 / 293 / 473 8/8 0.012
Bias is flat, so this is threshold margin rather than a defect.
Neither option previously proposed was expressible: the matrix budget is
nmax_per_dim*d for EVERY cell and --strict-samplers is per-SAMPLER, so "re-budget
this cell" and "drop this row from strict" both needed a mechanism that did not
exist. Adds CELL_BUDGET_MULT, in the same shape as the existing per-case
WARM_CASES budgets, and sets this cell to x4. x2 clears 8/8 but its MINIMUM
(105) is 5% above the floor -- not a margin worth trusting for a row that has
read 66 and 119 on unchanged code. x4 gives min 209 (2.1x) for ~4% of the gate's
evaluations, being one cell of ~96.
ADAPTIVE-ALLOC. Re-running the probe confirmation on the FIXED probe reverses a
claim I made repeatedly across #47/#51/#55: the `adaptive_alloc ON / d4_n1_s303`
row is a CONFIRMED opt-in regression, not realization noise. The earlier "not
confirmed" came from the probe's patching bug, which compared the flag against
itself and could not report "worse". At 5 fresh seeds the flag arm fails at
EVERY seed, frequently with HIGHER n_eff than the default arm -- so it degrades
posterior SHAPE, not efficiency. Opt-in, default off, never set by the pipeline,
so production is unaffected; recorded as FOLLOWUPS item 4 with the scoping work
needed before the flag could ever be promoted.
Also marks FOLLOWUPS items 1-3 resolved with their outcomes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Gate result
Worth stating explicitly, because it looks odd: both arms report the identical 355. That is correct and by design -- The probe now fails -- correctly, and this needs a decision
This is the probe working as intended for the first time. It is not caused by this branch -- the regression is in But it will now turn every subsequent PR red, because the flag is genuinely broken on that target and the probe no longer masks it. That is a policy call rather than something to fix here:
My preference is (2): it keeps the probe useful as a regression detector for the other four flag configurations, which currently all pass, and it makes the exclusion explicit and greppable rather than silently tolerated. (1) is the real fix but should not gate unrelated work. I have not done either -- say which and I will implement it. Everything else in the probe passes: |
| for nc in cfg["ncomps"]: | ||
| for ts in cfg["seeds"]: | ||
| for kind in samplers: | ||
| _mult = CELL_BUDGET_MULT.get((kind, d, nc, ts), 1) |
There was a problem hiding this comment.
The multiplier is applied only while main() builds jobs. The documented pytest entry point (test_shape_recovery.py:36) constructs its budget directly as _PRESET["nmax_per_dim"] * ndim, so RIFT_SHAPE_PRESET=standard pytest ... still runs this GMM cell at x1. Please centralize budget calculation in a helper and use it from both paths; otherwise the supposedly resolved starvation row remains inconsistently budgeted.
| _mult = CELL_BUDGET_MULT.get((kind, d, nc, ts), 1) | ||
| jobs.append((kind, (d, nc, ts), | ||
| cfg["nmax_per_dim"] * d, cfg["neff"], opts.run_seed)) | ||
| cfg["nmax_per_dim"] * d * _mult, cfg["neff"], opts.run_seed)) |
There was a problem hiding this comment.
This multiplier also applies after the user explicitly passes --nmax-per-dim, contradicting the CLI contract that nmax = this * ndim. For example, targeting this cell with --nmax-per-dim 200000 silently runs at 4.8M rather than 1.2M evaluations, which makes controlled x1/x2 measurements misleading. Please apply the override only to preset defaults, or expose and document a way to disable it.
|
Review summary: two P2 findings posted inline. (1) The per-cell override is bypassed by the documented pytest gate entry point. (2) It silently changes the meaning of an explicit |
…ling adaptive_alloc probe rows Two review findings on the CELL_BUDGET_MULT change: * The override was applied in shape_recovery.main() only, so the pytest entry point (test_shape_recovery.py, which the suite documents as an equivalent way to run the matrix) still used nmax_per_dim*ndim and left the very cell the table exists to fix starved. Both paths now go through one cell_budget() helper; verified they agree. * It also rescaled an EXPLICIT --nmax-per-dim, whose help text promises nmax = this * ndim. The x1/x2/x4 budget study that produced the x4 figure passes --nmax-per-dim, so it would have silently been measuring x4/x8/x16. An explicit --nmax-per-dim now disables the table and says so; --no-cell-budget-mult disables it at preset defaults. Separately, the confirmed --portfolio-adaptive-alloc shape regression (FOLLOWUPS item 4) is contained by commenting its two rows out of the probe, so the probe keeps working as a detector for the flags that pass. FLAG_CONFIGS hoisted to module level so the exclusion is greppable and so the confirmation-machinery tests inject a synthetic config list instead of coupling to whichever flags ship. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both review findings addressed, plus the follow-up decision on the failing probe rows (option 2: exclude). P2 — "the per-cell override is bypassed by the documented pytest gate entry point." Correct, and it defeated the point of the change: the matrix has two entry points ( P2 — "it silently changes the meaning of an explicit Verified by capturing what
adaptive_alloc probe rows — excluded, not tolerated. The two The exclusion is deliberately written as commented-out config lines so it is greppable, is recorded in FOLLOWUPS.md, and is asserted by
Validation. Probe on No RIFT library code is touched by this PR — the diff is the gate harness and its tests only — so a base-vs-candidate shape gate run would be comparing identical integrator code and is a no-op by construction. |
| until fixed. Pinned so the exclusion cannot be undone silently: reinstating those rows is the | ||
| first step of fixing the flag, and this test failing is the reminder that they will fail.""" | ||
| import inspect | ||
| src = inspect.getsource(PB.main) |
There was a problem hiding this comment.
This test is vacuous after FLAG_CONFIGS moved to module scope. inspect.getsource(PB.main) contains only configs = FLAG_CONFIGS, so it never contains portfolio_adaptive_alloc; uncommenting either known-bad entry in FLAG_CONFIGS would still pass. Please inspect PB.FLAG_CONFIGS directly, for example by asserting the key is absent from every active flags dict.
| cell_budget(kind, ndim, ncomp, tseed, _PRESET["nmax_per_dim"]), | ||
| _PRESET["neff"]) | ||
| ok, reasons = evaluate(r) | ||
| assert ok, "{} on {}: {}".format(kind, target.name, "; ".join(reasons)) |
There was a problem hiding this comment.
evaluate() returns status strings (PASS, FAIL, STARVED, or ERROR), all of which are truthy. Therefore this pytest gate passes even when the sampler fails or errors, undermining the entry point whose budget consistency this PR just fixed. Please unpack as status, reasons and assert status == "PASS".
|
Re-review summary: the original two findings are resolved. Two P2 test defects remain and are posted inline: the adaptive-allocation exclusion test inspects |
Two P2 test defects from re-review, each verified by reintroducing the defect: * test_adaptive_alloc_is_excluded_from_the_probe_configs inspected the SOURCE of PB.main(), which after the FLAG_CONFIGS hoist contains only `configs = FLAG_CONFIGS` -- so it could never see a re-enabled row. Confirmed vacuous: appending the known-bad config to FLAG_CONFIGS left the old form passing. It now inspects FLAG_CONFIGS itself, and additionally asserts the probe still has >= 2 opt-in arms, since an emptied list would satisfy "adaptive_alloc absent" while testing nothing. * test_shape_recovery.py unpacked evaluate() as `ok, reasons` and asserted `ok`. evaluate() has returned a STATUS STRING since 6467ac9 -- "FAIL", "STARVED" and "ERROR" are all truthy, so the pytest entry point has passed on every outcome it exists to catch since the day it was written. Verified: the old assertion passes on ERROR, STARVED and all four FAIL paths. On STARVED the reviewer asked for a strict `status == "PASS"`. Deviating deliberately: the gate defines STARVED as non-blocking in absolute terms and gating only differentially (6467ac9, after whole d=8 rows legitimately starved at production budgets), and strict equality immediately reds the DEFAULT preset -- GMM d4_n2_s101 reads n_eff=42 at quick's 200k budget. FAIL and ERROR assert; STARVED skips with the n_eff in the message, so it stays visible in the pytest summary and is not counted as a pass. Absolute-vs-base starvation gating remains compare_shape_results.py's job. Recorded as FOLLOWUPS item 5. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both P2s confirmed and fixed. I verified each by reintroducing the defect first, because a test that cannot fail is exactly what is under discussion. "The exclusion test is vacuous after I added a second assertion while there: "The pytest gate asserts a non-empty status string." Correct, and worse than it looks: that entry point has never enforced anything. Every status is truthy, so from 2026-07-22 onward
So the budget-consistency fix in this PR was making two entry points agree when one of them could not fail. Worth noting given that was the first finding. One deliberate deviation from the requested fix, on STARVED. You asked for
The skip carries the n_eff and says explicitly that it is not a pass, so it stays visible in the pytest summary rather than being silently absorbed: I am aware this leaves a hole: a branch that starves everything would skip everything and go green. I think that is acceptable because the merge gate is the differential run, not this one — but if you would rather close it, say so and I will either add a session-level guard that fails when more than some fraction of the matrix starves, or re-budget The under-budgeted Tests: 46 in |
Two things, both driven by measurements that finished overnight. The second corrects a claim I made repeatedly in #47, #51 and #55.
1. The mis-budgeted strict cell -- measured and fixed
GMM mix_d6_n3_s303sits on then_eff = 100starvation floor at the matrix budget, so as a merge-blocking row it is close to a coin flip on every branch. 8 fresh seeds per budget:Bias is flat across budgets, so this is threshold margin, not a defect in the cell.
Correction to the two options I offered earlier: neither was expressible. The matrix budget is
nmax_per_dim * dfor every cell, and--strict-samplersis per-sampler, not per-cell -- so "re-budget this cell" and "drop this row from strict" both needed a mechanism that did not exist. (DroppingGMMfrom strict would have dropped all 24 GMM rows.)This adds
CELL_BUDGET_MULT, in the same shape as the existing per-caseWARM_CASESbudgets, and sets this cell to x4. Why x4 and not x2: x2 clears 8/8, but its minimum is 105 -- 5% above the floor, which is not a margin worth trusting for a row that has read 66 and 119 on unchanged code. x4 gives min 209 (2.1x margin) and costs ~4% of the gate's total evaluations, being one cell of ~96.2.
--portfolio-adaptive-allocis a CONFIRMED regression on that targetRe-running the probe confirmation on the fixed probe reverses what I reported in three previous PRs. I described
adaptive_alloc ON / d4_n1_s303as pre-existing realization noise, "verified against the base." That verification ran on the buggy probe, which compared the flag against itself and structurally could not report "worse". It was never evidence.The flag arm fails at every seed, and frequently with higher n_eff than the default arm (292 vs 244, 177 vs 67, 140 vs 136). So it is failing on shape metrics -- more effective samples, worse recovered posterior. That is the "confidently wrong" signature documented elsewhere in this work (n_eff measures weight concentration, not coverage), now appearing in one of our own opt-in features.
Production is unaffected: the flag is opt-in, defaults off, and the pipeline never sets it. But it is not safe to promote, and the higher n_eff must not be read as evidence it helps -- that is exactly the reading that made the estimator-clip experiment look like a success while it was biasing lnZ by -11.5 nats.
Recorded as
FOLLOWUPS.mditem 4 with the scoping work needed: sweep the flag across the full matrix at several seeds, recording JS / pull / width alongside n_eff, to establish whether the allocation signal systematically over-concentrates on whichever member reports the best per-chunk n_ess. Deliberately not attempted here -- it is an investigation, not a gate-tuning change.3. Those two probe rows are now EXCLUDED from the probe
Follow-up to section 2, after review discussion. The
adaptive_alloc ONandadaptive+clip ONrows are commented out ofFLAG_CONFIGSinprobe_portfolio_optin_flags.py, so the probe keeps working as a regression detector for the flags that pass instead of becoming a permanently red row people learn to scroll past.This is containment, not a fix. The exclusion is written as commented-out config lines (greppable), recorded in
FOLLOWUPS.mditem 4, and asserted bytest_adaptive_alloc_is_excluded_from_the_probe_configsso it cannot be quietly lost. Reinstating those two lines is step one of any fix, and they will fail until the flag is actually repaired. Production is unaffected meanwhile -- opt-in, defaults off, pipeline never sets it.Probe on
d4_n1_s303with the reduced list:weight_clip ON,varaha floor .25,varaha band .25-.75,band + gmm cap3, allbase=PASS flag=PASS, 0 flagged rows.FLAG_CONFIGSwas hoisted to module level in the same change, because it had to be: the confirmation-machinery tests keyed onportfolio_adaptive_allocand, once it was removed, passed by having nothing left to flag. They now inject a synthetic config list rather than coupling to whichever flags happen to ship.4. Review fixes to the section-1 mechanism
Both P2 findings on
CELL_BUDGET_MULTwere right, and the second was more than cosmetic.The override was bypassed by the pytest entry point. The matrix has two of them --
shape_recovery.main()(whatrun_shape_recovery.shdrives) and the parametrization intest_shape_recovery.py-- and the first cut applied the table only inmain(). The one cell this table exists to un-starve was therefore still starved underRIFT_SHAPE_PRESET=standard pytest, which the suite documents as an equivalent way to run the matrix.It also rescaled an explicit
--nmax-per-dim. The CLI documentsnmax = this * ndim, and the x1/x2/x4 study that produced the x4 figure in section 1 passes--nmax-per-dim-- so it would silently have been measuring x4/x8/x16 under the labels x1/x2/x4.Both paths now go through one
cell_budget()helper; an explicit--nmax-per-dimdisables the table and prints that it did;--no-cell-budget-multdisables it at preset defaults. Verified by capturing whatmain()actually dispatches torun_one, rather than by recomputing the formula:GMM d6_n3_s303cell_budget()(pytest)main(), preset defaultmain() --nmax-per-dim 200000= npd*ndim, override offmain() --no-cell-budget-mult= npd*ndim, override offmain(), non-override celld4_n1Also
FOLLOWUPS.mditems 1-3 marked resolved with outcomes (probe confirm-on-fail #54,_rvscache audit #55, ordering constraint #57).Tests: 46 in
expensive_before_merging/integrators(28 of themtest_probe_confirm.py) + 36 intest/integrators(test_portfolio_restrict_and_warm22,test_replica_pooling9,test_rvs_weight_derivation5), all passing.On re-running the full gate: no RIFT library code is touched by this PR -- the diff is the gate harness, its tests and
FOLLOWUPS.md-- so a base-vs-candidate run would be comparing identical integrator code and is a no-op by construction. What a run would still show is the runtime cost of the x4 cell, which is the thing worth knowing before merge; happy to run it for that if you want the number, but it is not a correctness gate here.