AV: fix the high-SNR empty-live-volume crash and report the collapse; add the extrinsic zoom box - #171
Open
oshaughnessy-junior wants to merge 2 commits into
Conversation
Backport of rift_O4d PR #63 (d3dd2a2, 6a82836, f6bfc5d), reduced to the parts that are safe to carry onto a frozen production line. At high network SNR the extrinsic export died with ===> FAILED ANALYSIS <==== zero-size array to reduction operation maximum which has no identity Probable reasons: SEOB nyquist or starting frequency limit or signal duration The attribution was wrong: nothing about the waveform is involved. Measured on this branch, 12 replicates per arm, zero-noise injections at a fixed intrinsic point, production extrinsic config, rho_net 146.8 -- 11/12 exports crashed. A crashed export writes nothing, so the posterior is silently thinned in an SNR-dependent way: rho 51 -> 0%, rho 72 -> 3%, rho 103 -> 42%, rho 147 -> 92%. ROOT CAUSE. Two defects, stacked. 1. The production likelihood UNDERFLOWS: exp() of a lnL more than ~745 nats below the peak is 0 in float64, so it returns -inf. Over a cold extrinsic prior at rho 147 that is ~99996 of 100000 draws, so AV's live set is born holding a handful of samples -- sometimes one. 2. get_likelihood_threshold could then return a threshold >= max(lkl). With a small live set and one dominant weight BOTH of its terms degenerate: prob_stop_thr saturates at the MAXIMUM (every other weight underflows to exactly 0, so the discard_prob quantile IS the top sample) while lkl_stop_thr falls through the `len > nsel` test to the array MINIMUM. integrate_log applies that as a STRICT `allloglkl > loglkl_thr`, discarding at least one sample per cycle regardless of merit, so the live set ratchets to 1 and then to 0 -- and the empty array reaches `xpy_here.max(allloglkl)`. NOT CUPY-SPECIFIC. numpy raises the identical ValueError from the same line; the upstream traceback names CUPY_CUB_MAX only because production runs on the GPU. The reproduction on this branch raises the numpy flavour, and the new test reproduces it deterministically on the host path. CHANGES RIFT/integrators/mcsamplerAdaptiveVolume.py * get_likelihood_threshold: raise a named LiveVolumeCollapse on empty input, and CLAMP the threshold strictly below max(lkl) so it can never discard the entire live volume. A threshold at/above the max encloses zero probability, contradicting the enc_prob=0.999 the function exists to maintain. With the clamp in place at least the peak always survives, so nrec >= 1 is guaranteed and the empty reduction downstream is unreachable. * live_volume_collapse_verdict() + ess_from_log_weights(): report a degenerate contraction instead of silently exporting one sample. dict_return (which was empty on this branch) gains live_volume_collapsed / collapse_reason / n_live_final / n_ESS, and the run prints an [AV COLLAPSE] block. bin/integrate_likelihood_extrinsic_batchmode * the "Probable reasons: SEOB nyquist ..." hint was printed for EVERY exception in the block. Make it conditional and name the integrator collapse when that is the cause. This misattribution is why the failure went undiagnosed. WHY THE VERDICT IS WORTH CARRYING ONTO A FROZEN LINE. The clamp stops the crash, after which the run that used to crash completes and exports -- sometimes from a single surviving sample. Unreported, that converts a silent THINNING (a crashed export is visibly missing) into a silent CONTAMINATION (a degenerate export looks ordinary to downstream posterior assembly), which is worse. The verdict is purely additive: it reads log_wt after the integration loop and no sampling decision consumes it. WHY THE VERDICT DOES NOT GATE ON k-hat. k-hat exceeds its nominal 0.70 "unresolved tail" threshold even in HEALTHY runs on this problem -- the converged rho=51.4 replicates measure 0.819-1.605 -- so gating on it would false-flag good exports. ESS separates the regimes with nothing in between (16.3-34.3 healthy vs 1.0-2.0 collapsed), so the verdict keys on ESS and live-set size. DELIBERATELY NOT PORTED from upstream, to keep the change small on a production branch: the RIFT_AV_TRACE per-cycle instrumentation, the integrate_log restructuring (finite screening, empty-cycle recovery, previous-state restore), the machine-readable _integrator_status.json sidecar, the ILE --reject-collapsed-live-volume gate, and the warm-seed affine-rank rule. None is needed to stop the crash measured here; all of them change more of the sampling loop or the output contract than this branch should take. TESTS. test/test_av_empty_live_volume.py, a focused backport of the upstream 910-line suite: 24 passed, 2 GPU-only skips. Mutation-tested -- with the clamp disabled, 5 tests fail, including the two that drive integrate_log to the exact production error ("zero-size array to reduction operation maximum which has no identity"). test_clamp_is_inert_when_the_live_set_is_well_populated reproduces the PRE-FIX threshold formula verbatim and asserts the clamped code returns the identical value on a healthy 20000-sample live set, so the clamp cannot silently shift a production lnZ. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Backport of rift_O4d PR #58 (af68d7e zoom-box options, f715f4f the cosine-sampler fix, 7e6f2ed the GPU-safety + CI review round). This branch had no --limit-* options at all, so the port brings the whole feature in its already-fixed form rather than introducing the defect and then repairing it. WHAT THE OPTIONS DO. --limit-right-ascension / --limit-declination / --limit-inclination / --limit-psi restrict the extrinsic sampling AND prior ranges to a truth-centered box, the way --d-min/--d-max already do for distance, so the adaptive sampler can resolve a narrow high-SNR peak it could never find from the full prior. THE DEFECT THIS PORT AVOIDS. Upstream, the parsed limits were stored in param_limits[...] as radians but only the plain (angle) branches consulted them: the --declination-cosine-sampler and --inclination-cosine-sampler branches hardcoded left_limit=-1, right_limit=1 and never looked at param_limits. No error, no warning, no narrowing -- the run simply sampled the whole sphere / whole iota range. The cosine samplers are the production default in the extrinsic-export config, so a requested sky box silently collapsed to RA-only narrowing. THE TRANSFORM. The limits are always given in radians of the PHYSICAL angle and are mapped into the coordinate actually sampled. The conventions are read off the likelihood closures in the same file (dec = pi/2 - arccos(z), iota = arccos(z)): declination: sampled variable is z = sin(dec). sin() is INCREASING on [-pi/2, pi/2], so the limit order is PRESERVED: [lo,hi] -> [sin(lo), sin(hi)]. inclination: sampled variable is z = cos(iota). cos() is DECREASING on [0, pi], so the limit order SWAPS: [lo,hi] -> [cos(hi), cos(lo)]. The naive [cos(lo), cos(hi)] yields an INVERTED, empty sampling interval; the test suite asserts against exactly that. PRIOR SEMANTICS ARE PRESERVED. The cosine branches keep the FULL-RANGE constant prior density 1/2 in z (previously they reused the sampling pdf object as the prior, which was the same thing only because the range was [-1,1]). It is deliberately not renormalized over the box, so the prior mass removed by a box is 0.5*(z_hi - z_lo), identical to the non-cosine branch where prior_pdf is 0.5*cos(dec) resp. 0.5*sin(iota) over the same physical box. The isotropic prior is therefore unchanged and the two samplers give the same posterior and the same lnZ for the same physical box. ALSO IN THIS CHANGE * The non-cosine dec/iota branches use a properly TRUNCATED sampling pdf and cdf_inv when a limit is given: setting left/right_limit while keeping the full-range dec_samp_vector / cos_samp_vector would let mcsampler / mcsamplerGPU (which, unlike the AV sampler, do use pdf and cdf_inv) draw outside the box. With no limit requested the legacy functions are used unchanged. * The truncated closures infer their array module from the argument they are handed (infer_array_module), because mcsamplerGPU.draw_simplified() calls them with a SINGLE positional cupy array; an explicit xpy= still wins. * Limits are validated: a malformed 'LO,HI', an empty or inverted range, or a range that does not overlap the physical domain now fails loudly instead of producing a degenerate sampler. Dec/iota ranges are clipped to [-pi/2,pi/2] resp. [0,pi]. * Fixed an adjacent silent bug in the scalar time-marginalized likelihood closure, which zipped the RAW sampled `inclination` instead of the converted `incl`; under --inclination-cosine-sampler that fed cos(iota) to the waveform as an angle. This branch carried the same bug. KNOWN LIMITATION, CARRIED ACROSS DELIBERATELY. --limit-right-ascension / --limit-declination together with --internal-sky-network-coordinates is now a HARD ERROR: that option samples RA/dec in a rotated, network-aligned frame, so an equatorial sky box would silently select the wrong patch of sky. Failing loudly beats boxing the wrong patch, but it forecloses 2-IFO boxing, where the ring is narrow in the NETWORK frame and that is exactly where a box would help most. Follow-up work, not addressed here. New helpers live in RIFT/integrators/mcsampler.py (infer_array_module, clip_angle_limits, cosine_sampler_limits, ret_dec_samp_vector, ret_dec_samp_cdf_inv_vector, ret_cos_samp_vector, ret_cos_samp_cdf_inv_vector) so they are importable and testable independently of the backend the run selects. The block is byte-identical to upstream and purely additive. TESTS. test/test_limit_cosine_samplers.py, ported unchanged from upstream: 29 passed. Covers the dec order-preserving and the iota order-SWAPPING transform; clipping; empty/inverted/non-finite ranges raising; the truncated samplers reducing to the legacy ones over the full range; identical restricted support and identical prior mass in both samplers; AV-style (volume x prior) equivalence and identical posterior shape; end-to-end MCSampler.integrate agreement between the two parameterizations for both a flat and a peaked integrand; backend inference and an end-to-end mcsamplerGPU.draw_simplified() draw inside the box; and a source-level guard against reintroducing the hardcoded [-1,1] range. The order-swap is mutation-tested: flipping the `reverses` flag on the inclination convention fails 11 of the 29 tests, including test_inclination_limits_map_to_cos_and_SWAP_order. Both new suites are wired into .travis/test-integrate.sh. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
oshaughnessy-junior
had a problem deploying
to
private-review-dispatch-rift-upstream
August 13, 2026 07:40 — with
GitHub Actions
Failure
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.
Two backports from
rift_O4d, both measured onrift_O4citself rather than inferred.1. The crash (
000cf79b)On a loud, well-localized source the extrinsic-export ILE dies with
The stated reason is wrong -- the generic handler attributes any exception to waveform
generation -- and the failure is not GPU-specific; the host path fails identically.
Mechanism. A double-precision
expof a lnL more than ~745 nats below the peak underflowsto zero, so at high amplitude almost every cold draw over the extrinsic prior comes back
-infand the live set starts with a handful of members. With few live points and onedominant weight,
prob_stop_thrsaturates atmax(lkl)(every other weight has underflowed,so the discard_prob quantile IS the top sample) while
lkl_stop_thrfalls back to thesmallest. The resulting threshold is applied downstream as a strict
lkl > thr, so each cyclediscards at least one point regardless of merit; the live set ratchets to one, then to zero,
and the next reduction is over an empty array.
Fix. Clamp the threshold strictly below the largest live value (
np.nextafter), so it cannever discard the whole live volume, and raise a named
LiveVolumeCollapserather than lettinga reduction fail. In a healthy run
len(lkl) >> nselandlkl_stop_thris the nsel-thlargest, far below the max, so the clamp never engages.
Measured on rift_O4c at rho_net = 146.8, 12 replicates, cold production config:
4a8703f3(this branch's base)The collapse report ships with the clamp, deliberately
The clamp alone would be a downgrade in one respect. A run that used to crash now completes
and exports -- at
eff_samp=1.00, fairdraw=1, a one-sample "posterior". Unreported, thattrades a silent THINNING (a crashed export writes nothing and is visibly missing from the
count) for a silent CONTAMINATION (a degenerate export looks ordinary to CIP). So the branch
also carries the
[AV COLLAPSE]report: ~40 additive lines after the integration loop, readby no sampling decision. It fires on either of two geometric conditions -- the final live set
holding no more points than the dimension (it cannot span the space, let alone describe a
posterior in it), or Kish ESS < 2 (fewer than two effective samples is one sample) -- and
prints the ESS and the live-set size. Pareto k-hat is deliberately not used as a gate: it
exceeds its nominal 0.70 threshold even in the healthy runs on this problem (the converged
rho=51.4 replicates measure 0.819-1.605), so it would have false-flagged good exports. ESS
separates the regimes with an order of magnitude and nothing in between (16.3-34.3 healthy
vs 1.0-2.0 collapsed).
Inert where the integral already converges
The gate for a frozen production line. Same host, same GPU, sequential, 20 replicates each
at rho_net = 51.4, comparing distributions (seed-matching cannot work across code versions:
the clamp changes the sequence of threshold decisions, so the sample stream diverges):
No detectable change in behaviour. The patched arm flags 2/20 as collapsed; the baseline has
an exact equivalent it does not flag (
fairdraw=1, eff_samp=1.11), so those are the sameevent reported, not a behaviour change.
Worth noting from that control: 3 of 20 baseline runs at rho=51.4 came back at fairdraw <= 2
(eff_samp 1.1-2.0). The collapse is not a switch that flips above rho ~ 72; it is a tail that
exists at moderate amplitude and grows. Today those runs are exported unflagged.
2. The extrinsic zoom box (
ca9c4249)rift_O4chas no--limit-*options at all, so this brings the feature in its already-fixedform (rift_O4d PR #58 plus its GPU-safety follow-up) rather than introducing the defect and
repairing it.
--limit-{right-ascension,declination,inclination,psi}restrict extrinsicsampling AND prior. The transforms:
--declination-cosine-samplerissin(dec)(closures do
dec = pi/2 - arccos(z)), so[lo,hi] -> [sin lo, sin hi], order preserved;cos(iota), and cos is decreasing on[0,pi], sothe order swaps:
[lo,hi] -> [cos hi, cos lo].Prior mass is identical under both parameterizations, so lnZ is unchanged for the same
physical box. Tests cover both transforms, clipping, empty/inverted ranges raising, reduction
to the legacy samplers at full range, and end-to-end
integrateagreement; the inclinationorder-swap test is mutation-tested (flipping the reverse flag fails it). A run with no limit
given is bit-identical.
While porting,
rift_O4cwas found to carry the same latent bug the O4d branch fixed: thescalar time-marginalized closure passed the raw sampled
inclination(i.e.cos iotaunderthe cosine sampler) as an angle, while
decwas converted correctly in the same loop. Fixed.Known limitation, carried across deliberately:
--internal-sky-network-coordinatesplus asky box is a hard error, because an equatorial box selects the wrong patch of a rotated frame.
That forecloses 2-IFO boxing, where the ring is narrow in the NETWORK frame -- the right fix is
to interpret the limits in that frame. Follow-up work, not attempted here.
Scope
The clamp stops exports from vanishing; it does not make the loudest events correct. At
rho ~ 147 the patched exports are degenerate one-sample draws -- now flagged rather than
silent. Boxing converges for rho <~ 100. Above that, measurements on
rift_O4dput standaloneAV ~15 nats high in lnZ against an AV+GMM portfolio with a comparable peak search, so the
portfolio is the answer there; that is separate work (see PR #168 for the fair-draw backend it
needs on this branch).
Validation harness, all measurements and the reproduce commands:
RIFT_roboto_paper/analyses/extrinsic_collapse_demo/(run/O4C_INERT_CONTROL_2026-08-13.md,run/O4C_PORT_STATUS_2026-08-11.md).