DRAFT/RFC: AV grid freeze at the threshold freeze — bounds the block runaway, but a reference integral says it adds bias - #96
Draft
oshaughnessy-junior wants to merge 3 commits into
Conversation
Once at_final_threshold is True the truncation budget is spent: the threshold
never moves again, every drawn sample is kept, so nrec == ninj and the live
volume V is constant. But the grid kept being rebuilt from
delta_V = V/sqrt(nrec), and nrec keeps growing simply because samples
accumulate -- so prod(nbins) = 1/delta_V climbed without bound, more bins
became occupied, and
ninbin = n_chunk // n_occupied + 1
floored to one draw per bin as soon as n_occupied > n_chunk. From there the
per-cycle block IS the occupied-bin count and grows without limit; the only
ceiling is nmax. Production reached a block of 149,543, which the CPU ILE
likelihood turns into ~67.6 GB of host allocation.
The in-source comment claimed this "asymptotically becomes stationary". It
does not: it is positive feedback (more samples -> finer grid -> bigger block
-> more samples). test_av_grid_freeze.py is the counter-example, on a thin
curved shell in 5-D -- the geometry that matters, since occupied bins then
grow with resolution rather than saturating as they do for a round blob.
Fix: hold nbins/dx at the resolution they had on the first cycle at the final
threshold. V, dx0, the estimator and the GPU path are untouched, and before
the freeze the new branch is never taken.
Both copy-paste twins are patched. The one in update_sampling_prior_selfish
is UNREACHABLE as the code stands -- that method re-initializes trunc_p on
every call and takes a single step, so at_final_threshold is always False
there -- but letting the twins diverge is how this defect survived in the
other one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
self.nbins/self.dx are rebound rather than mutated on the normal path, but handing out the stored array itself means any future in-place write to self.nbins would silently corrupt every later cycle. A 6-element copy per cycle costs nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The comment claimed refining after the freeze is 'inconsistent with what the estimator claims'. That is too strong. n_occupied/prod(nbins) is a bin COVER of the live region, so exceeding V is expected, not a contradiction; and the drift is not by itself a bias, because integrate_log re-applies the threshold to the accumulated set every cycle, which keeps the retained samples uniform on the live region however loose the cover gets. What is true, and all that is claimed now: it is a second quantity this code implicitly treats as settling down which measurably does not (factor 3-6.7 drift over the post-freeze cycles, exactly constant with the freeze). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
oshaughnessy-junior
deployed
to
private-review-dispatch-rift
August 14, 2026 17:24 — with
GitHub Actions
Active
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.
1. The defect
integrate_logcontracts the live volume by raising a likelihood threshold, spending atruncation budget
trunc_pas it goes. Once that budget is spent,at_final_thresholdbecomes True — and stays True, because
trunc_pstops being updated. From then on:nrec == ninj;V *= (nrec/ninj)is a no-op — the fractional live volume is constant.But the grid is still rebuilt every cycle from
nreckeeps growing for the trivial reason that samples accumulate. So the grid keepsrefining after the threshold has stopped moving, more distinct bins become occupied, and
once
n_occupied > n_chunkthe//floors to one draw per bin — from there theper-cycle block is the occupied-bin count, and it climbs with no ceiling but
nmax.More samples → finer grid → bigger block → more samples.
The in-source comment on that block says it "asymptotically becomes stationary". It does
not. Measured, in production: a block of 149,543, which the CPU ILE likelihood
(
DiscreteFactoredLogLikelihoodViaArrayVectorNoLoop, then_cal == 1/xpy is npbranch) turns into ~67.6 GB of host allocation — calibrated at
peak RSS = 0.45 GB + 4.49e-4 GB x block. Those jobs were killed by a 64 GB cgroup limitand stuck permanently (
HoldReasonCode34/21; the autofix ladder tops out at 65536 andperiodic_releasecovers only codes 45/13).2. Reproduction, in six seconds, without any of that
test/test_av_grid_freeze.pyintegrates a thin curved shell in 5-D. The geometry isthe point: for a shell the occupied-bin count grows with resolution like a surface area,
which is what pushes
n_occupiedpastn_chunk. A round Gaussian saturates instead anddoes not reproduce the defect — worth knowing before someone "simplifies" the target.
On the base commit, per-cycle block over one run at
n_chunk=400:Block ×3 and still rising when the budget ran out. With the patch it is exactly
constant across the whole post-freeze tail while
nreckeeps growing.3. The change
Hold
nbins/dxat the resolution they had on the first cycle at the finalthreshold.
V,dx0, the estimator (log_joint_s_prior), and the GPU path areuntouched, and before the freeze the new branch is never taken.
The state is reset in
setup(), whichintegrate_logcalls at the top of everyintegration, so a frozen grid cannot leak between calls (asserted by a test).
Both copy-paste twins are patched. The one in
update_sampling_prior_selfishisunreachable as the code stands — that method re-initialises
trunc_p = 1e-10onevery call and takes a single step, so
at_final_thresholdis always False there. It ispatched anyway because letting the twins diverge is how the defect survived in the other
one; if
trunc_pis ever made persistent so that path can freeze, it will already becorrect instead of silently acquiring the same runaway. This is a claim you can check by
inspection, and it means the second hunk changes no number today.
Deliberately not included: the prototype also carried a
np.maximum(nbins, 1)guardagainst
np.floorproducing a zero bin count. That is a real (if unreachable-looking)edge case, and it is a separate change.
4. Blast radius — measured, and smaller than "every AV run"
The change can only do anything to an integration that reaches the threshold freeze and
then keeps cycling. Across every instrumented integration in this campaign
(S240919bn + S241127aj, production container):
Measured. The main loop terminates on its
n_efftarget before the truncation budgetis spent, so the patch is inert there in every run observed. Argued: a main loop that
burns its budget without reaching
n_eff— which does happen on hard events — wouldreach the freeze, so this is a property of these runs, not a theorem.
4a. "What if the threshold freezes while the grid is still coarse?"
The obvious objection: freezing at the wrong moment would hold a bad resolution for the
rest of the run. Two answers.
Structural (argued, checkable by inspection):
at_final_thresholdis sticky — theonly place
trunc_pis updated is insideif not(at_final_threshold), so once it is Trueit can never flip back. The freeze therefore engages once and never chatters.
Empirical (measured, 267 slice integrations that reached the freeze):
nrecIt never fires on the first cycles; the volume has always contracted for at least 7 cycles
first. It can fire early relative to the whole run (as low as 2% of the way through),
which is exactly the runaway case the change exists to bound — and there the measured
effect on convergence is an improvement, not a loss: minimum n_eff across replicates
went 2.96–5.19 (control) → 10.34–17.41 (freeze), with the median unchanged.
5. Validation
test/test_av_grid_freeze.py: 6 passed on this branch; 5 of 6 fail onthe base commit (the sixth is a sanity floor on the integral, which passes either
way). It asserts the invariant — grid and block stop changing after the freeze, while
the live set keeps growing — not a wall-clock or a memory number.
test/test_av_empty_live_volume.py(the PR AV: fix the high-SNR empty-live-volume crash, and report the collapse #63 gate): 58passed, 2 skipped, unchanged.
that never reaches the final threshold, base and branch agree to the last digit
(
lnI = -9.4325475738906377,ln var,eff_sampandntotalall identical). On atarget that does freeze, they differ — as they must. Measured.
RIFT/integrators/TESTING.md: 1 blocking row(
AV mix_d6_n3_s101, lnZ bias +0.337 vs a 0.317 threshold). Re-tested at 5 fresh seedsper arm as the suite requires: NOT CONFIRMED (2 worse / 3 not-worse) → 0 confirmed
blocking regressions, gate passes. But see 6f: the unconfirmed row is not
uninformative.
6. Does it move the integral? — settled, and not in this change's favour
This was the open question. It is now answered by a brute-force reference integral, so
the answer does not depend on believing AV.
6a. The reference
More AV replicates cannot settle this: they only shrink the scatter around whatever AV
converges to. So I built an independent estimator of the same integral,
I = int L(Omega) p(Omega) dOmega, at one pinned distance (d = 800 Mpc):qis frozen before any sample that enters the estimate is drawn: afour-component defensive mixture — an importance-weighted fine histogram of the
pilot, a dilated uniform cover at the same resolution, an importance-weighted coarse
histogram, and uniform-on-the-box — all piecewise constant, so
qis exactly evaluableand exactly sampleable.
mean(L p / q). For a fixedq > 0this is exactlyunbiased for any
q, good or bad; proposal quality controls only the variance. Theuniform-on-the-box term makes
q > 0unconditional, so no part of the support can besilently missed however bad the pilot was.
Validated against 5-D integrals with closed-form answers, including from a deliberately
mismatched pilot and from a pilot that misses half the support: recovers the exact value
to <= 0.01 nats.
Three configurations, different bin resolutions and different seeds, 4e6 draws each:
Reference: lnI = 125.637 +/- 0.004 (chi2/dof = 0.12, total spread 0.009). All k-hat
well below 0.7, so the error bars mean what they say.
6b. The verdict
Every d = 800 slice integration in the campaign, pooled by configuration:
Measured. At block 2000 the control sits on the reference and the freeze sits ~0.3 nats
above it. The +0.48 nat shift is therefore bias INTRODUCTION, not bias removal. The
argument that the shift was probably a correction does not survive.
At block 10000 — the configuration PR #64 deployed, and what production runs today —
both arms sit on the reference, the freeze arm to +/-0.016 nats. Whatever the freeze
does there is at most a few hundredths of a nat.
6c. A ground-truth target agrees, and says it does not vanish at block 10000
The shell target of section 2 has a computable exact answer (
lnI_exact = -9.767505).24 paired seeds per arm, same seeds in both arms:
Measured. AV is already biased high on a thin shell (pre-existing, not caused here),
and the freeze adds to it — same sign at every chunk size, closer to truth in 15 of
96 paired seeds. Note the last row: raising the chunk to 10000 shrinks the base bias by
3x but leaves the freeze's contribution essentially unchanged at ~+0.02 nats.
So the block-10000 null in 6b is a statement about precision, not about the effect
being zero: +/-0.016 nats cannot exclude a +0.02 shift. The two are consistent with "the
freeze adds ~0.02 nats everywhere, visible above the noise only when the chunk is small".
6d. The corollary that reframes the change
If the refined grid is more accurate, the post-freeze refinement is not purely
pathological: it buys accuracy while destroying the memory behaviour. This is a
trade-off, not a free bug fix — precisely the thing a reviewer, not a patch author,
should decide.
6e. Two arguments I am withdrawing
The self-consistency argument. It said the occupied-bin union shrinks after the
freeze while
Vis held fixed. Measured from the per-cycle logs asln(V_geom/V)withV_geom = n_occupied/prod(nbins), it does the opposite — the cover drifts looser,by a factor 3-6.7, and is exactly constant under the freeze:
And
V_geom > Vis expected anyway — a cover exceeds what it covers — while a loosecover is not itself a bias, because the threshold is re-applied every cycle, keeping the
retained samples uniform on the live region. What survives is only: a quantity the code
implicitly treats as stationary drifts without bound, and the freeze makes it stationary.
The size of the block-2000 shift. An independent replicate set at d = 800 with an
identical harness for both arms (7 control / 10 freeze) gives +0.14 +/- 0.21, not
+0.48 +/- 0.06. Same sign, but the +/-0.06 was optimistic.
6f. What the merge gate's unconfirmed row was telling us
The gate found one blocking row (
AV mix_d6_n3_s101) and the required 5-fresh-seed re-testreturned NOT CONFIRMED (2 worse / 3 not-worse), so it does not block. But the
underlying numbers are one-sided in a way the verdict does not capture: n_eff was lower in
the freeze arm in 5 of 5 seeds (96/116, 112/129, 113/135, 128/138, 133/145) and every
quoted lnZ bias was positive (+0.370, +0.410, +0.366). A second AV cell
(
mix_d8_n1_s202) also came back with an upward bias, +0.598.Not blocking — but it pointed the same way as 6b and 6c, on a third target, before those
were measured.
7. Design questions
7.1 Unconditional, behind a flag, or not at all? — not as it stands
I started this expecting to argue for unconditional. The measurements moved me off it, and
the order matters, so here is the reasoning as it actually went.
The case for unconditional, which is still sound on its own terms:
believed the grid became stationary; the runaway is not a policy anyone chose.
TESTING.mdrequires a flaggedchange to pass the default-path gate and be probed with the flag on — so a flag costs
more review, not less, and leaves the defect live by default.
ladder that cannot reach the memory they ask for.
integrations affected, and section 5 shows bit-identity wherever the freeze never
engages. Production AV runs are not seeded —
run_shape_recovery.shexplicitly forbidspinning sampler seeds, because independent copies that localise differently are the
working detector for mode collapse — so production numbers already vary run to run.
Why none of that is enough. Every one of those arguments is about blast radius and
review cost. None survives contact with section 6: against a reference integral the
change makes the answer worse — +0.30 nats at block 2000 (3.7 sigma), where the control
sits on the reference — and a ground-truth target puts the effect at ~+0.02 nats even at
block 10000, one-sided in 96 of 96 paired seeds.
Recommendation: do not merge this as written, in either form. Unconditional ships a
known bias; a flag ships the same known bias behind a switch, which is worse, because it
neither fixes the default nor forces the trade to be confronted. What the branch is ready
for is review of the mechanism, the reproduction and the measurements.
The three things I would do instead, in order of how much I would argue for them:
must be fixed now, and the distance-slice path can cap the per-cycle block as a
scheduling decision, with no effect on AV's proposal, volume bookkeeping or estimator.
That is a different PR and I have not measured it — but note it must NOT be done by
subsampling
binunique, which was already measured and rejected (n_eff 6.44 against atarget of 15; see 7.2).
for a bounded block is a defensible bargain — 0.02 nats is far below the per-slice
sigma_lnL of ~0.12 — but it is a call for the person who owns the systematics budget,
made in the open, not smuggled in as a bug fix.
the interesting question section 6d exposes and none of this work answers.
On scoping it to the slice path (a fourth option): I argue against it. The mechanism is
in AV; the slice path is merely where
block x per-sample bytesfirst crossed a cgrouplimit. Section 6c reproduces the bias on a synthetic target with no slice path in sight.
7.2 Is freezing the grid the right shape? — right shape, wrong effect
Argued, with one measured rejection.
binunique— measured and rejected: n_eff 6.44against a target of 15. It fails structurally, not by tuning: the occupied set is the
proposal's support, so drawing from a random subset each cycle samples a different
sub-region every cycle while the estimator still asserts uniformity over the whole live
volume. Any alternative must avoid this.
prod(nbins)(equivalently floordelta_V) at a constant. Nearly equivalent ineffect but requires justifying a number. This is the comparison that made freezing
attractive in the first place: the freeze introduces no new constant — the resolution
is chosen by the algorithm's own state, at the moment its own stopping condition fires.
That property is still true and still worth something; it is just not sufficient.
nrec). Changes the estimator,not the schedule. Much larger blast radius.
delta_Vinstead ofnbins. Identical under the default isotropic split;under anisotropic binning it would fix the total bin budget while letting the per-axis
allocation keep moving. A genuine alternative, unmeasured. See 7.3.
7.3 Interaction with the warm-start / portfolio machinery
Argued, from reading; not measured.
portfolio_varaha_never_freezeconcerns theportfolio's member-update schedule, not the likelihood-threshold freeze. It routes
through
update_sampling_prior_selfish, whereat_final_thresholdis always False(section 3), so the grid freeze never engages in a portfolio member. Unrelated
mechanisms with confusingly similar names.
bootstrap_from_samples/_build_grid_from_pointsbuild the seeded grid outside thecycle loop and are untouched.
setup()clears the freeze state before the warm state isapplied, so a seeded run cannot inherit a stale frozen grid.
trunc_pthat is already final, makingat_final_thresholdTrue on cycle 1. The new branch is not taken (nothing stored yet), socycle 1 behaves exactly as before and its resolution becomes the frozen one. This
exposes a pre-existing wart worth a separate look:
integrate_logrecomputesnbinsoncycle 1 regardless, so a warm start's chosen
nbins_per_axisis discarded on the firstcycle with or without this patch.
_allocate_nbins(anisotropic). It redistributes the same total budget(
prod(adaptive) = 1/delta_V), so anisotropic binning reproduces the runaway exactly anddoes not mitigate it; freezing works identically. The residual concern is that freezing
also freezes the anisotropy. Post-freeze the live set only densifies within a fixed
region, so the per-axis spreads should stabilise — but that is an argument, not a
measurement, and
anisotropic_binsis off by default. Open.8. Relationship to #64 — and why #64 matters more than it looked
#64 raised the slice block from a hardcoded 2000 to the ILE chunk size (10000). That fixed
convergence quality (n_eff p50 17.97 vs 15.37, frac(n_eff≥10) 0.999 vs 0.954 over
101,350 production slices) but left a 2.0% memory tail, because
nsel = min(1000, 0.1*n_chunk)saturates at 1000: raisingn_chunkdelays the runaway, it cannot removeit. This change was aimed at that residual tail. Section 6b adds something #64 did not claim
for itself: at block 2000 the control arm sits on the reference and at block 10000 it sits
on it more tightly (-0.025 +/- 0.043), so #64 is not only a convergence-quality fix, it
is the configuration in which AV's slice integral is measurably unbiased at this distance.
Whatever is done about the memory tail should not disturb that.
Not proposed, because all three were measured on this problem and were worse:
--internal-sky-network-coordinates(8/15 slices burned budget vs 4/15), anisotropicbinning (41,123 occupied bins vs 26,379 isotropic), and a sky prior box.
9. Open items
which does not touch the estimator. Unmeasured; the obvious naive version (subsampling
binunique) is already known to fail.is the interesting question here.
delta_V-freeze vsnbins-freeze under anisotropic binning (7.2, 7.3) — unmeasured.np.floor(nbins) -> 0edge case (section 3) — untouched here.should be, before anyone leans on section 6b for a different configuration.
10. Reproducing any of this
pytest test/test_av_grid_freeze.py(~9 s).RIFT/integrators/TESTING.md; base vs branch, thencompare_shape_results.py --confirm-base-checkout ... --confirm-cand-checkout ... --confirm-repeats 5.the campaign scratch area; the reference is an env-gated
RIFT_DSLICE_BRUTEhook infresh_sample_slicesplus abrute_reference.pymodule. Say the word and I will cleanthem up into
test/expensive_before_merging/— they are more generally useful than thisPR, since they give AV a ground truth to be scored against.
🤖 Generated with Claude Code