Sync current oshaughnessy-junior rift_O4d after #173 consolidation - #178
Draft
oshaughnessy-junior wants to merge 168 commits into
Draft
Sync current oshaughnessy-junior rift_O4d after #173 consolidation#178oshaughnessy-junior wants to merge 168 commits into
oshaughnessy-junior wants to merge 168 commits into
Conversation
NOT FOR MERGE. One worked example of the proposal in DESIGN_rvs_naming.md, wired into a single sampler, so the shape can be argued about against something concrete instead of against prose. Nothing reads it: this branch is a no-op on every output. THE PROBLEM. sampler._rvs means the RETAINED SET while integrate_log accumulates, and an EXPORT RESAMPLE afterwards. The name does not change, the type does not change, so a consumer written against the first meaning keeps working -- silently -- against the second. THE EVIDENCE THAT THIS IS DESIGN AND NOT LUCK. Nine defects of this shape. Five before the audit (CIP export; L0 seed #78; reject gate #79; reserve cap and its logarithm #84), three found by the mechanical sweep in #87, and then FOUR MORE IN REVIEW OF THAT FIX -- every one of the four in the boolean bookkeeping introduced to describe _rvs from outside, none in the physics: 1. a fix correct in isolation, wrong once pooling ran after it 2. one flag answering two questions (rows-resampled vs globally-equal-weight) 3. the CLI option used where "what this pass actually did" was needed 4. a marker cleared only on the normal return, surviving a raised event Each was a second source of truth that some site touching the first failed to maintain. That is what a naming problem looks like once you refuse to rename anything. WHAT THIS DRAFT CONTAINS. RvsRecord carries rows AND provenance together, and replaces the booleans with named questions -- rows_are_resampled() (per BLOCK, survives pooling), is_equal_weight() (whole RECORD, pooling destroys it), blocks_were_flattened() (a fact about the pooling STEP). Those are the three the flags kept conflating; they are three methods with three names because the failure was never that the answer was hard to compute, it was that one name suggested one question while a caller asked another. Provenance is per-BLOCK, so a mixture of raw and resampled replicas is representable at all. The suite is written as one section per review-round failure shape: each test would have caught its round had provenance lived with the rows from the start. If the design is adopted they justify it; if not, they are the specification any replacement has to satisfy. Deliberately NOT a dict subclass: that would let every existing sampler._rvs[...] keep working against an object whose meaning it does not check, which is the original problem with more steps. Consumers reach for .columns, which is visible in a diff and greppable by the audit. BLAST RADIUS, measured by audit_rvs_fairdraw.py: 306 reads, 131 post-rebind, 7 rebind sites. That is why this is option A (two names, incremental) rather than option B (the fair draw returns a new object), which is the correct end state but cannot be done in one change to code that writes science products. Both are written up, with the trade-offs and three open questions, in DESIGN_rvs_naming.md. Verified no-op: 132 passed, 3 skipped across the AV/L0/portfolio suites, and a collapsed AV pass records n_retained=288 against 1 exported row -- the case the whole line of work is about.
…t the LISA question
1. NAMING -> _rvs_record, underscored per review. Local to the sampler, even though the goal
is to standardise the concept across integrators.
2. RETAINED ROWS -> measured, and the answer differs by sampler, which the question did not
anticipate. measure_retained_set_memory.py, run with no fair draw so _rvs IS the retained set:
AV ~0.9 MB per million nmax -> ~4 MB at nmax=4e6
portfolio ~91.6 MB per million nmax -> ~384 MB at nmax=4e6
They differ because AV keeps only the in-volume subset, which grows far more slowly than
ntotal, while the portfolio's _rvs holds EVERY draw, so its cost tracks nmax directly. 384 MB
per ILE process is a real operational cost when many ILE jobs share a node.
RECOMMEND NOT holding the raw retained set unbounded. The portfolio's is mostly ballast: on
the collapsed pass this work is about, the finite fraction is ~1e-5, so nearly all of that
384 MB is -inf rows no consumer can use. make_warm_seed_reserve already keeps a bounded,
finite-stratified copy with the exact pre-cap weight total -- so have the record REFERENCE the
reserve rather than take its own copy, and treat full retention as an AV-only opt-in where it
costs ~4 MB. That gets the value #79's lnZ fallback needs at a cost already being paid.
3. LISA -> the question was badly posed and implied something untrue. There is NO separate
integrator: both drivers import the identical set (mcsampler, Ensemble, GPU, AdaptiveVolume,
Portfolio), so _rvs_record reaches LISA for free and there is no LISA-side decision here.
The divergence is the DRIVER: integrate_likelihood_extrinsic_batchmode_lisa is 2,526 lines
against the main driver's 4,563, a fork of an older ILE with ZERO occurrences of
ln_weights_from_rvs, _pool_replica_rvs, _lnZ_of_rvs, _kish_neff_of_rvs, the L0 rescue, the
sequential warm start, replicas, .dgrid or the proposal breadcrumb. So LISA has no consumer to
migrate -- its 36 post-rebind reads are the MAP-seed/export pattern, already BENIGN/PER_ROW in
the ledger. The real issue is two forks of one ILE, one of which silently misses every fix.
That is a separate and larger problem, noted so it is not mistaken for this one.
125 passed, 2 skipped. Still a no-op: nothing reads the record.
The --check gate merged in #87 caught this draft's new sampler-side _rvs read on the very next change to touch one -- which is the behaviour it was built for, on a case nobody wrote it for. Verdict PER_ROW: the record takes the just-rebound columns as a VIEW plus the pre-draw row count, and reads no statistic of them. It records that they ARE the export resample, at the moment that becomes true.
#95 Reviewer: "B sounds super dangerous ... flag as in plan for longer-term, but not anytime in the next month or two." Agreed, and recorded where the reasoning will be found rather than in a PR comment: option B is parked in issue #95 with the measured blast radius (306 reads, 131 post-rebind, 7 rebind sites) and a definition-of-done, and the doc now says so at the option itself as well as in the recommendation. A stays the direction, and with the memory question settled the concrete next step is to have the record REFERENCE the existing bounded reserve rather than take its own copy -- ~4 MB for AV but ~384 MB for a portfolio at nmax=4e6, most of it -inf ballast no consumer can use. Also notes that A is what makes B cheap later: once consumers ask a record instead of indexing a dict, B becomes a change of what the default view returns rather than a 306-site rename.
Agreed direction from review. Deliberately one worked example rather than a sweep, so the shape can be judged before the mechanical part. RESERVE BY REFERENCE, not a copy. retained_points()/retained_lnL()/n_retained() point at the bounded, finite-stratified _warm_seed_reserve. From the measurement: holding raw retained rows costs ~0.9 MB per million nmax for AV (nothing) but ~92 MB per million for a PORTFOLIO, i.e. ~384 MB at nmax=4e6 per ILE process -- and it would be mostly ballast, since the portfolio's finite fraction on the collapsed pass this work is about is ~1e-5. The reserve already keeps the affordable thing, with the exact pre-cap weight total so a capped reserve still yields an unbiased lnZ. A pooled record carries NO reserve: it is a mixture of several passes, so there is no single retained set, and pointing at one arbitrary pass's would be worse than None. AV RECORDS BOTH PATHS. fair_draw when the draw fires, retained when it does not. "Absent" and "not resampled" are different statements, and a consumer forced to distinguish them is back to combining conditions by hand -- which is the failure this design exists to remove. POOLING BUILDS A POOLED RECORD carrying _rep_fairdraw PER BLOCK. That is precisely what the two booleans cannot express, and why a raw/resampled mixture needed a special case in _pool_replica_rvs; the record represents it directly. FIRST CONSUMER MIGRATED: ln_weights_for_posterior, chosen because it is the exact site of the one-flag-two-questions defect, so converting it demonstrates the point instead of merely exercising the API. It trusts a record only when `.columns is rvs` -- _rvs is a mutable dict that may have been replaced since the record was built -- and otherwise falls back to the flags. KEEPING TWO DESCRIPTIONS HONEST is the real cost of A, and four review rounds on #87 were all "two descriptions drifted apart", so it is asserted rather than promised: * the record and the flags agree across retained / fair draw / pooled / pooled-mixed / pooled-raw; * on a real collapsed AV pass the record path and the flag path return BIT-IDENTICAL weights, on both branches -- the conversion is a refactor, and stays checkable until the flags go. 26 record tests; 213 passed, 3 skipped across the integrator suites; --check green at 134 sites (it caught the new sampler-side read again, now classified).
…mplers set it
STEP 2 -- the remaining consumers. .dgrid and the extrinsic-proposal breadcrumb already went
through ln_weights_for_posterior, so they moved with it; the .dslice guard and the pooled n_eff
now ask the record directly. Note each asks a DIFFERENT question, which is the point:
.dslice rows_are_resampled() -- survives pooling; reweighting resampled rows
double-counts whether or not they were pooled
pooled n_eff blocks_were_flattened() -- a fact about the pooling STEP, and keying it on
either other question is what made that branch
dead code in review round 2
weights is_equal_weight() -- whole-record
All three go through ONE lookup, _rvs_record_for(sampler, rvs), which declines a record whose
.columns is not the dict being held: _rvs is replaced in place, so "the sampler has a record"
and "the record describes these rows" are different questions. The PRODUCER at the pooling site
asks a third -- _sampler_keeps_records -- and has its own name rather than an exemption, because
it is about to replace sampler._rvs and would otherwise be told "no record" and silently skip
building the pooled one.
STEP 3 -- all seven rebind sites, wired by one patcher against PR #87's own markers so they are
identical rather than seven hand edits. Each site now resets the record, builds a `retained`
record before the draw, and replaces it with a `fair_draw` record after. The reserve rides
along by reference where the sampler keeps one (AV, portfolio); None elsewhere is the honest
answer rather than a gap.
TWO THINGS FOUND DOING IT, both recorded in the design doc:
* n_retained HAD TO BE CAPTURED EAGERLY. RvsRecord.retained(self._rvs) holds a reference to the
live dict, which the draw then rebinds, so len(record) afterwards returns the POST-draw count.
Reading it made a collapsed pass report n_retained == rows -- "nothing was discarded", the
exact opposite of the truth. This project's own bug class, in the code written to prevent it.
Caught because the end-to-end check printed n_ret == rows and that looked wrong.
* mcsampler and mcsamplerEnsemble take a LINEAR integrand; AV and the portfolio take log. The
wrong kind makes the fair draw compute negative weights and raise. Confirmed to fail
IDENTICALLY on the pristine file before concluding anything, so it is a harness contract, not
a defect I introduced.
Tests: 31 in the record suite, including every wired sampler agreeing with its flags on both
draw settings, and a structural check that all seven sites are wired the same way (one patcher
means one mistake would be replicated everywhere -- the case worth testing rather than eyeballing
a diff). 248 passed, 3 skipped overall; --check green at 141 sites.
Still DRAFT. The flags stay until every consumer is migrated; deleting them is step 4.
… fix it
Raised in review: the backends are structurally different per backend, which is a landmine for
developers. Agreed, and it is a SEPARATE problem from the naming one -- RvsRecord does not
address it -- so the first step is to stop it being invisible.
audit_backend_contracts.py records what each backend actually does, and --check (now in CI)
fails when one CHANGES without the recorded table changing with it. It deliberately does not
forbid the differences: several are load-bearing, and none should be "tidied" without a
decision. It makes a change show up as a diff instead of as a wrong number months later.
WHAT IT FOUND, and it is worse than the "log vs linear" I first assumed. _rvs['integrand']
holds THREE different things:
linear L mcsampler, mcsamplerGPU
lnL (aliased) mcsamplerAdaptiveVolume, mcsamplerNFlow, mcsamplerPortfolio
L *or* lnL mcsamplerEnsemble, depending on the return_lnI kwarg
The last is the dangerous one: for that backend the column's meaning is a RUNTIME property of
how the pass was called, so reading the consumer cannot tell you which it is. That is exactly
why ln_weights_from_rvs demands use_lnL explicitly and why it must be the STORED convention
rather than opts.internal_use_lnL -- a constraint that was already documented at that function
but nowhere discoverable from the backends themselves.
The failure is asymmetric, which is what makes it a landmine rather than a nuisance: a log
callable into a linear entry point makes the fair draw compute NEGATIVE weights and raise; the
same mistake downstream does NOT raise, it takes log() of a log and returns a plausible,
almost-flat weight vector. It cost time twice in one afternoon wiring the record, which is the
only reason it is written down rather than rediscovered.
Two more differences recorded because consumers must cope with them: only AV and the portfolio
keep a _warm_seed_reserve (so retained_points() answers None for the other four), and the
portfolio's _rvs holds EVERY draw against AV's retained subset -- ~92 vs ~0.9 MB per million
nmax, so n_retained means different things per backend.
Verified the gate by removing NFlow's integrand aliasing: it reports the exact field that moved
and the exact before/after, then passes again on restore.
251 passed, 3 skipped; both gates green (141 _rvs sites, 6 backend contracts). Still DRAFT.
Adds _sinc_Q_window_numpy alongside _nearest_/_cubic_Q_window_numpy, plus a
self-contained accuracy test. Not yet wired to time_interp; default behaviour is
unchanged.
Q^a_lm(t) is band-limited to fmax and sampled at 1/deltaT, so the figure of merit
is the oversampling factor fNyq/fmax. The two stencils fail differently, and
NEITHER is uniformly better -- which is the point of this commit:
fNyq/fmax nearest cubic sinc a=8 sinc a=32
1.5 3.6e-1 6.2e-2 1.2e-3 9.9e-5
2 2.8e-1 2.7e-2 7.9e-4 4.7e-5
4 1.7e-1 2.2e-3 4.3e-4 2.8e-5
8 5.6e-2 9.0e-5 2.7e-4 2.0e-5
16 4.8e-2 1.0e-5 3.3e-4 2.2e-5
cubic is a 4-point Lagrange polynomial: O(h^4), so it improves fast with
oversampling and is poor near Nyquist. sinc is window-limited, so its error
PLATEAUS -- independent of oversampling. Crossover is around fNyq/fmax ~ 4-8.
This matters because PRODUCTION RUNS NEAR NYQUIST: srate 4096 with fmax ~1700 is
fNyq/fmax ~ 1.2, where sinc is 50x more accurate than cubic. A heavily
oversampled configuration is the opposite case and should keep using cubic.
I went in expecting sinc to be a general improvement. It is not, and the test
asserts the crossover in BOTH directions so that a later "fix" making sinc win
everywhere fails loudly -- it would mean the window had been widened until the
stencil was no longer local.
Default stays 'cubic': the right choice depends on fNyq/fmax, which the stencil
cannot see. Cost is 2a taps against 4, so term1 scales ~a/2.
test_q_window_interp.py is numpy-only (no LAL, no data), ~1 s.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…uidance on the flag
Wires the stencil added in the previous commit, and closes two pre-existing holes
the survey turned up.
VALIDATION. factored_likelihood.py had the only hard gate
("time_interp must be 'nearest' or 'cubic'"); it now defers to a shared
validate_time_interp(). factored_likelihood_with_rotation.py and
factored_likelihood_freqresponse.py had NO validation at all -- their dispatch is
`if nearest: ... else: <cubic>`, so ANY unrecognised value silently ran cubic.
Both now validate. That is a bug fix independent of this feature.
GPU. There is no Q_inner_sinc kernel in cuda_Q_inner_product.cu, so 'sinc' on GPU
raises NotImplementedError rather than falling back -- a silent fallback would
misreport which stencil produced a number. cal_method='fused' was already gated
generically on time_interp != 'nearest', so it needs nothing.
CLI. --interpolate-time now takes 'nearest'|'cubic'|'sinc' as well as the legacy
truthy value (still meaning cubic), so existing invocations and the
--internal-ile-interpolate-time plumbing are unchanged. The choice guidance lives
in the help string, where the choice is actually made: which stencil is right
depends on fNyq/fmax, sinc is 35-50x better at 1.2-2, cubic ~30x better at 16,
crossover 4-8, and typical production (srate 4096, fmax ~1700) sits at ~1.2.
REFACTOR. The four CPU nearest/cubic branches now go through
_q_window_numpy_interp() instead of each open-coding the two-way choice, so a
fourth stencil is one edit, not four.
VERIFIED
* test_q_window_interp.py PASSES (asserts the crossover in both directions).
* test_slowrot_noloop.py PASSES unchanged: baseline-vs-rotation max|diff|
3.638e-12, matching the documented figure, so the dispatcher refactor is
behaviour-preserving for nearest and cubic.
* guards fire: unknown value -> ValueError; sinc+GPU -> NotImplementedError;
dispatcher routes each name to its stencil and sinc differs from cubic.
NOT DONE (draft): no GPU kernel; no end-to-end ILE run with 'sinc'; the JAX stack
(jax_ile/core.py _GATHERERS, --interp) has its own nearest/linear/cubic set and is
untouched.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…w weighting family The two ILE drivers are a deliberate fork (RO, 2026-08-13: "the overhead of one ring to rule them all is too high"). This does not argue with that. It makes the consequence -- drift -- mechanically visible, so it stays a choice. Measured on junior/rift_O4d @ 364a22f: main is 4,883 lines, lisa 2,526, and 132 items (helpers, CLI options, module constants, sampler provenance markers) exist in main and not in lisa. Both drivers import the SAME integrators and expose an identical ok_lnL_methods, so all of that drift is in the driver. THE AUDIT. audit_lisa_driver_drift.py diffs the two by AST across FUNC / OPTION / CONST / ATTR. Two extraction traps worth recording: the drivers use optparse, so an argparse-only scan reports zero options; and the provenance readers use the getattr(obj,'name',default) form, which is a Call and not an Attribute, so a naive scan reports a real port as a no-op. Both are handled. THE LEDGER. make_lisa_drift_ledger.py holds the judgements as ordered family rules -> lisa_drift_ledger.json. 132/132 classified: PORT 70, NA 43, PHYSICS 11, PORTED 8. An item matching no rule is reported and fails --check; that fired for real once, on --sampler-anisotropic-bins. "Does not apply to LISA" is a fine answer; silence is not. THE PORT. The three consumers PR #87 actually fixed -- the proposal breadcrumb, .dgrid, and the .dslice core -- do not exist in this driver, so there was no live w^2 bug here. The hazard did exist: the driver sets igrand_fairdraw_samples from --fairdraw-extrinsic-output, and all seven shared rebind sites already set _rvs_is_fairdraw, so the marker was arriving and nothing read it. Ported ln_weights_from_rvs, ln_weights_for_posterior, _rvs_is_export_resample, _rvs_is_equal_weight, _rvs_len, _rvs_lnL_convention and the marker reads. The trap avoided: --internal-use-lnL is also accepted for adaptive_cartesian_gpu and portfolio, which set use_lnL WITHOUT return_lnI and still store linear L. The stored convention is therefore derived from pinned_params['return_lnI'], never the option. Deliberately NOT done, both recorded at the site: ln_weights_for_posterior passes use_lnL through UNRESOLVED exactly as main does (a latent trap in both drivers -- a same-named helper behaving differently across the fork would be worse); and _truthy_option was moved out of this family once its only caller turned out to be the --interpolate-time normalizer, so porting it would have been dead code. TESTS. test_lisa_fairdraw_weights.py (29) including an anti-drift test pinning each ported helper AST-identical to main's, docstrings excluded. Revert-checked: six mutations, each caught by its named test, file restored byte-identical. test_lisa_driver_drift.py (7) is the gate, revert-checked both directions. Both wired into the lisa-check CI job. That job already ran nine LISA test files and stayed green through all 2,357 lines of this drift, because all nine are import/contract/smoke level. The gate does not test the physics; it refuses to let a new item through without a recorded human decision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Q_inner_sinc joins Q_inner and Q_inner_cubic in cuda_Q_inner_product.cu, so
time_interp='sinc' no longer raises NotImplementedError under --gpu.
The tap weights are NOT re-derived in CUDA. _sinc_lanczos_weights is refactored
into _sinc_lanczos_weight_matrix(u, a, xpy=np), a vectorized, backend-generic
form; the cupy wrapper evaluates it with xpy=cupy so the weights are built on the
device (no host round trip for the per-sample offsets, which at production
n_extrinsic would move tens of MB per detector per call) from the SAME source
expression the CPU window builder uses. A CPU/GPU disagreement is then
unambiguously a kernel bug, not a re-derived-formula bug. The refactor is
numerically inert: test_q_window_interp.py reproduces its table exactly
(1.246e-3 / 7.852e-4 / 4.259e-4 / 2.692e-4 / 3.254e-4 at fNyq/fmax 1.5-16).
The four GPU dispatch branches (factored_likelihood x2, _with_rotation,
_freqresponse) now route through one _q_inner_product_gpu helper mirroring the
CPU _q_window_numpy_interp, so a future stencil cannot be wired into three sites
and forgotten in the fourth. validate_time_interp's GPU guard is dropped.
MEASURED on an RTX 2080 Ti (sm_75; the develUWM cupy 10.6/CUDA 11.2 cannot
target the Blackwell cards on pcdev11/13 -- nvrtc rejects -arch sm_120):
new test_q_window_interp_gpu.py, max|GPU-CPU| / scale
weights numpy vs cupy backend 3.331e-16
interior nearest/cubic/sinc 4.3e-17 / 2.7e-17 / 7.2e-17
edge/zero-ext nearest/cubic/sinc 5.7e-17 / 6.2e-17 / 1.4e-16
test_slowrot_gpu.py (Path B), max|diff| lnL 7.276e-12 for all three
test_slowrot_freqresponse_gpu.py (Path D) 5.5e-12 / 7.3e-12 / 9.1e-12
The edge case is the one that discriminates: the sinc stencil is 16 taps wide,
and the weights are normalised over the FULL stencil BEFORE out-of-range taps
are dropped -- dropped taps are not renormalised away. A kernel that
renormalised the survivors, or that let a negative index wrap, still looks
perfect on interior windows.
test_slowrot_gpu.py and test_slowrot_freqresponse_gpu.py now loop over all three
stencils rather than ('nearest','cubic').
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
--internal-ile-interpolate-time was action='store_true' and appended a literal
'--interpolate-time True', i.e. always cubic. It now takes an optional value:
bare (or the legacy 'True') means CHOOSE, an explicit nearest|cubic|sinc is
passed through untouched. Existing invocations keep working, just smarter.
THE THRESHOLD IS MEASURED, not chosen by taste. Re-running the accuracy harness
with 12 seeds per point (ratio = cubic error / sinc error, >1 means sinc wins):
fNyq/fmax 3 4 4.5 5 5.5 6 7 8
ratio 10.4 3.4 2.3 1.4 0.91 0.77 0.50 0.26
so the crossover is at fNyq/fmax ~= 5.3, and the seed-to-seed spread brackets
1.0 only over 5-6. The threshold is set to 5, deliberately on the CUBIC side:
through the ambiguous band the two errors are within ~30% of each other while
sinc costs ~4x cubic in the Q product, so there the cheaper incumbent wins.
Production (srate 4096, fmax 1700 -> 1.2) gets sinc; the slow-rotation
brute-force configuration (fmax 512 at srate 16384 -> 16) gets cubic.
The decision lives in a new numpy-only leaf module
RIFT/likelihood/time_interp_choice.py rather than in the helper, for two
reasons: importing factored_likelihood into the helper would cost ~4 s of numba
compilation per workflow build (measured 4.36 s vs 0.87 s for lalsimutils), and
a threshold buried in a script cannot be unit-tested. test_time_interp_choice.py
covers the threshold band, both real configurations, malformed-input fallback,
and the legacy 'True' spelling.
AUDITABILITY: the helper writes the RESOLVED stencil name onto the ILE command
line (never the literal 'True') and logs srate, fmax, fNyq/fmax and the choice,
so a completed run's stencil is readable off the .sub file instead of being
re-derivable only by replaying the helper. The ILE driver echoes the resolved
stencil at startup too.
Also closes a silent-failure path found while wiring this: the driver mapped any
unrecognised --interpolate-time value to 'nearest' via the truthiness test, so a
typo ('sinK', 'lanczos') silently changed the likelihood's time discretization
and looked exactly like a run that never asked for interpolation. Unrecognised
values now raise. That matters more now that the helper writes stencil NAMES.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The stencil width and first-tap offset were read back off the cupy offsets array; indexing a device array for a Python int forces a sync, once per detector per likelihood call. Both are known from halfwidth, so derive them host-side and assert the shape agreement instead. Parity numbers unchanged (test_q_window_interp_gpu.py: 7.167e-17 interior, 1.428e-16 edge, for sinc). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ipline Pass 2 of the catch-up. Closes 16 of the 124 remaining gap items (gap 124 -> 108). WHY THIS FAMILY FIRST. The rescue targets the high-SNR n_eff LOTTERY: a large fraction of independent AV/portfolio runs collapse to n_eff ~ 1 by contracting onto the wrong spot, and the rescue re-seeds such a run from the peak it did find. LISA MBHB are high-SNR by construction, so that is the regime and not an edge case. The sampler-side machinery (build_warm_seed, lnZ_from_reserve, the reserve itself) already lives in RIFT/integrators/ and so already reached LISA; only the driver wiring was missing. Ported verbatim: _lnZ_of_rvs, _kish_neff_of_rvs, _lnZ_of_reserve_or_rvs, _snapshot_pass_state, _restore_pass_state, _warm_seed_reserve_for, _warm_seed_geometry, _clear_warm_state, the _warm_seed_reserve marker, and seven options whose defaults and help text are kept IDENTICAL to the main driver's -- including --sampler-l0-rescue-reject-dlnZ 3.0, which is a MEASURED value (the old 0.5 binned 25% of good portfolio warm passes while catching 0 of 55 truncated ones). A test pins the defaults in both files, because a knob that means something different in the two drivers is worse than a missing one. ONE DELIBERATE STRUCTURAL DIVERGENCE. The main driver inlines the rescue in its single analyze_event. This driver has TWO -- analyze_event_LISA (--LISA) and analyze_event (the fallback) -- each with its own integrate call and export block, already ~50% duplicated. Inlining twice would create a third copy to keep in step, which is the failure mode this whole exercise exists to prevent. So the block lives in _maybe_l0_rescue and both call it. That is a divergence in SHAPE, not behaviour, and it buys something the main driver does not have: the audit records that in main these call sites "cannot be exercised from a unit test" because analyze_event needs data, PSDs and a waveform. Here the gate is a function of its arguments, so 20 of the new tests drive the reject logic directly -- including the reject path, the accept-truncated override, the raising-warm-pass path and the mixed-provenance fallback. ORDERING IS LOAD-BEARING, and differs between the drivers. In main the `if not(res): raise` guard sits ~200 lines below the integrate call, so the rescue lands before it by accident of layout. Here that guard is immediately after integrate, so the rescue had to be inserted BETWEEN them: a degenerate early termination returns (None,None,None,None) and is the STRONGEST rescue trigger, so raising on it first would skip exactly the case the rescue exists for. Pinned by a test that locates both call sites and asserts integrate < rescue < guard. TESTS. test_lisa_l0_rescue.py (45), wired into the lisa-check CI job. Revert-checked with 11 mutations -- ordering, both Finding-5 reserve paths, the snapshot alias, both restore paths, the column-order guard, the provenance fallback, the measured default, the non-swallowing clear, and the degenerate trigger. Each caught by its named test; file restored byte-identical. One of the 11 came back WEAK on the first run and the test was rewritten: the mixed-provenance case had been set up with numbers where the correct and broken paths both accepted the warm pass, so it passed with the guard disabled. It now uses values where the two paths disagree about the outcome. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… "CPU-only"
The tree said 'sinc' costs ~a/2 = ~4x cubic in the Q product. That is the tap
ratio (16 against 4) and it holds on the CPU, but it is wrong for the new GPU
kernel, which is bandwidth/latency bound. Measured, ms per Q-product call:
GPU (RTX 2080 Ti), cubic -> sinc, at (n_ex, window, n_lms, n_time)
(1e4, 50, 5, 4096) 0.83 -> 1.35 1.6x
(4e4, 50, 5, 4096) 2.95 -> 5.41 1.8x
(1.6e5,50, 5, 4096) 11.39 -> 20.66 1.8x
(4e4, 100, 9, 8192) 6.11 -> 18.08 3.0x
CPU window builder, cubic -> sinc
(2000, 50, 5, 4096) 243 -> 1093 4.5x
(8000, 50, 5, 4096) 973 -> 4378 4.5x
(8000, 100, 9, 8192) 1371 -> 5730 4.2x
Also fixes a claim that went stale with the GPU kernel: the --interpolate-time
help still told users 'sinc' was CPU-only with no GPU kernel.
Crossover wording in _sinc_Q_window_numpy updated from the eyeballed "~4-8" to
the 12-seed measurement (~5.3, ambiguous over 5-6), and pointed at
time_interp_choice, which is what now acts on it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e gates in CI
BASELINE NoLoop had no GPU coverage: test_slowrot_gpu.py and
test_slowrot_freqresponse_gpu.py exercise Paths B and D, but
DiscreteFactoredLogLikelihoodViaArrayVectorNoLoop has TWO device dispatch sites
of its own (n_cal==1, and the n_cal>1 calibration 'loop' path) and neither was
tested. test_noloop_gpu_stencils.py covers both, all three stencils, against a
real PrecomputeLikelihoodTerms (the n_cal banks come from actual
calibration_realizations, 3% amplitude / 30 mrad phase, not a copied buffer):
max|GPU-CPU| on lnL nearest cubic sinc tol
n_cal=1 7.816e-14 1.563e-13 1.492e-13 1.069e-08
n_cal=4 calmarg loop 7.816e-14 1.563e-13 1.563e-13 1.068e-08
Tolerance fixed a priori as 1e-8 + 1e-11*max|lnL|; observed is ~5 orders under.
Parity alone can be vacuous, so two structural guards ride along: the three
stencils must give DIFFERENT GPU lnL (nearest-vs-cubic 9.69e-1, cubic-vs-sinc
2.46e-2), and the count of calls into _q_inner_product_gpu must be n_det for
n_cal=1 and n_det*n_cal for the loop path -- which also proves the loop path
hands the kernel its 819-sample block slice, not the 3276-sample concatenated
buffer.
test_calmarg_stencil_gating.py covers the fused-vs-loop gate: cal_method='fused'
raises NotImplementedError for 'cubic' and 'sinc' and runs for 'nearest', where
it agrees with the loop reduction to 1.243e-14. The DRIVER-level gating is
verified by ast-parsing integrate_likelihood_extrinsic_batchmode, extracting the
cal_method/cal_distmarg keyword expressions at all 7 NoLoop call sites, and
evaluating them over the full (gate) x (stencil) truth table: no site can route
cubic or sinc to the fused kernel. Stated plainly in the file: those
expressions are evaluated, the surrounding control flow is not.
CI: test_q_window_interp.py collected ZERO tests under pytest -- every assertion
lived in main(), so `pytest` on it reported success while running nothing. That
is the file holding the two-directional crossover gate. Assertions moved into
test_ functions (unchanged in content; main() still works), and a new
q-window-stencil-check job runs it and test_time_interp_choice.py. No CI job
referenced any likelihood test before this.
Also drops the last stale "'sinc' is CPU-only for now" from the NoLoop docstring.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…low persistence Pass 4 of the catch-up (pass 3, MC-error replicas, deferred). Closes 14 of the 108 remaining gap items (gap 108 -> 94). Pure pass-through to samplers this driver ALREADY wires: it exposes the same ok_lnL_methods as the main driver and builds mcsamplerPortfolio the same way, and its portfolio setup block was byte-identical to main's apart from the missing kwargs. Before this the knobs were reachable only through --sampler-portfolio-args, an eval-able dict; the pipeline passes the named flags. Option definitions are copied verbatim, and a test asserts default/type/action/choices match the main driver's for all 14. A knob that means something different in the two drivers is worse than a missing one: the same pipeline command line would otherwise produce two different integrations. The freeze-policy assembly is inline in both drivers rather than a function, so the tests extract the block and exec it against a fake opts -- testing the real source rather than a paraphrase. That covers the property the assembly exists for: an option left UNSET must stay out of the dict so the sampler keeps its own default, and 0 (which disables probing/reviving) is a REAL value that truthiness would silently drop. NF flow load/save go through _maybe_load_nf_flow / _maybe_save_nf_flow for the same reason the L0 rescue did: this driver has TWO analyze_event variants, and wiring only one would be a silent half-port. A test asserts both get both hooks and that they straddle the integration in the right order. TESTS. test_lisa_sampler_plumbing.py (55), wired into lisa-check. Revert-checked with 7 mutations: a drifted default, a dropped `is not None` guard, inverted VARAHA precedence, a dict that never reaches setup(), a lost hasattr guard, a misplaced save hook, and an unguarded NF exception. The hasattr mutation came back WEAK and the test was rewritten. Asserting "does not raise" was worthless there: the body is wrapped in `except Exception`, so dropping the guard still does not raise -- it announces "loading pre-trained flow", calls a method that does not exist, and swallows the AttributeError, so every non-NF run would log a flow load that never happened. The test now asserts the hook stays SILENT for a sampler with no flow support. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…w to check it
Re-ran test_q_window_interp_gpu.py and test_noloop_gpu_stencils.py on an RTX PRO
4000 Blackwell (sm_120) as well as the RTX 2080 Ti (sm_75). Every parity number
is identical to the last digit across the two architectures:
weights numpy vs cupy backend 3.331e-16
interior nearest/cubic/sinc 4.273e-17 / 2.724e-17 / 7.167e-17
edge/zero-ext nearest/cubic/sinc 5.720e-17 / 6.205e-17 / 1.428e-16
NoLoop n_cal=1 and n_cal=4 loop, all three stencils, max|GPU-CPU| <= 1.563e-13
so the kernel is not architecture-sensitive and the earlier sm_75-only evidence
was not a special case.
Getting cupy 10.6 onto Blackwell at all needs a two-part workaround, documented
in the test docstring because anyone re-running these on this fleet will hit it:
cupy computes min(arch, nvrtc_max_cc) on STRINGS, so min("120","86")=="120" and
it hands nvrtc 11.2 an sm_120 it cannot target. CUPY_COMPILE_WITH_PTX=1 plus a
sitecustomize pinning _get_arch to "86" fixes it; either alone still fails. That
is a test-time workaround, not a production recommendation.
The remote reads were checksum-gated against the local worktree first (shared
NFS $HOME is not instantly coherent, and a stale read produces a false green that
looks exactly like a real pass).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…collapse gate Pass 5a of the catch-up. Closes 5 of the 94 remaining gap items (gap 94 -> 89): --sampler-save-state, --sampler-load-state, --sampler-anisotropic-bins, --reject-collapsed-live-volume, and _reject_if_collapsed. All four are sampler-agnostic. The saved state is the AV sampler's own live-volume grid, which carries no detector convention; the bin allocation is per-axis on that same grid. THE ONE THING TO CARRY FORWARD. The main driver calls its collapse gate TWICE -- on the first run AND on the replica pool, because replication can turn a healthy first run into a collapsed POOL, and gating only the first would silently bypass the flag for exactly the case pooling introduces. This driver has no replica pooling yet, so only the first call exists here. That is recorded in the helper's docstring, in the drift ledger, and in a test that asserts the warning is still written where whoever ports --mc-error-replicas will be working. The helpers are hoisted to module level rather than nested (as _reject_if_collapsed is in main), because this driver has TWO analyze_event variants and nesting would mean two copies. A test pins the hoisted body AST-identical to main's nested one. THIS EXPOSED A FALSE POSITIVE IN THE DRIFT AUDIT, now fixed. It compared FUNC items by QUALIFIED name, so main's analyze_event._reject_if_collapsed did not match this driver's correctly-hoisted top-level _reject_if_collapsed, and the item would have sat in the gap forever no matter how well it was ported. A gate that cannot be satisfied is a gate people learn to ignore. FUNC items are now matched on the bare name as well. TESTS. test_lisa_av_state.py (27), wired into lisa-check. Revert-checked with 8 mutations: the lost AV-method restriction on save, bins not reaching portfolio members, bins ceasing to be opt-in, a gate that ignores its flag, a gate that fires on healthy runs, a collapse that is no longer announced, the gate moved before the not(res) guard, and deletion of the second-call-site warning. Each caught by its named test; file restored byte-identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The cost of the extra taps is not the same on the two backends -- measured
~4.2-4.5x cubic on CPU (the window builder is tap-count bound, 16 against 4) but
only ~1.6-3.0x on GPU (Q_inner_sinc is bandwidth/latency bound, so the taps are
largely hidden). Cost is what breaks the tie through the band where the two
stencils are within tens of percent of each other, so the tie should break in a
different place on each backend. A single threshold was answering a CPU
question for a GPU run.
Re-measured the crossover at finer resolution, 24 seeds x 8 targets per point
(ratio = cubic error / sinc error; frac = fraction of seeds where sinc wins):
fNyq/fmax 4.0 4.5 5.0 5.25 5.5 5.75 6.0 6.5 7.0
ratio (med) 3.52 2.08 1.43 1.23 0.95 0.85 0.75 0.62 0.44
frac 1.00 1.00 0.92 0.88 0.38 0.08 0.04 0.04 0.00
Median crossover is 5.4; sinc wins in EVERY realization up to 4.5 and
essentially never above 5.75. So:
GPU 5.5 -- sinc costs only ~2x, so let accuracy decide: put the threshold at
the measured crossover.
CPU 5.0 -- sinc costs ~4.5x, so only pay it while the advantage is robust
rather than marginal (median 1.43x, 92% of seeds) instead of out to the
point where it is a coin flip.
The gap is deliberately small and that is itself the result: the accuracy curves
are steep through the crossover, so a 2x difference in cost moves the optimum by
only ~0.5 in fNyq/fmax. Production (fNyq/fmax ~ 1.2) is far from either
threshold, so this does not change the production answer -- it only matters for
oversampled configurations near the crossover.
choose_time_interp_stencil gains on_gpu and now also returns the threshold it
applied, so the helper's log line names the backend and the value rather than
hardcoding one. The helper reads the same flag that gates its own
'--vectorized --gpu' append.
test_time_interp_choice gains a test that the GPU threshold is the LOOSER one
(the ordering follows from the cost ratio, so an inversion means the cost
measurement was misread) and that there is a regime where the backend actually
changes the answer -- otherwise the distinction would be decorative and should be
deleted rather than maintained.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…uction util_RIFT_pseudo_pipe.py passes --propose-ile-convergence-options unconditionally, and that is the flag gating both the '--vectorized --gpu' append and (now) the backend choice. So everything built through the normal pipeline takes the GPU threshold; the CPU one is reached only by invoking the helper directly without that flag -- in which case the helper also emits no --vectorized --gpu, and --interpolate-time needs the NoLoop path those select. The previous comment described the flag correctly but left the impression that both branches see production traffic. They do not. Say so, and say why the CPU value is kept anyway: the cost asymmetry behind it is measured and real, and a future CPU workflow should not silently inherit a GPU-shaped tradeoff. No behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…al audit
An adversarial audit of the two previous commits found one crash and a set of tests that
did not test what they claimed. Fixes, in severity order.
CRITICAL -- the rescue killed every --sampler-method adaptive_cartesian run.
_maybe_l0_rescue opened with
_neff_val = None if neff is None else float(sampler.identity_convert(neff))
copied verbatim from the main driver, where it also sits BEFORE the guard. That is safe
there only by luck: identity_convert comes from MCSamplerGeneric, and
RIFT.integrators.mcsampler.MCSampler -- the object this driver keeps for
--sampler-method adaptive_cartesian -- does not inherit it (verified by instantiation).
The line is the first statement of the function, outside the try and before the option
guard, so it ran on every event whether or not the rescue was enabled: AttributeError at
the END of a completed integration, before --output-file is written, losing the whole
point's compute. adaptive_cartesian is one of the five documented ok_lnL_methods.
Fixed by asking whether the rescue APPLIES before touching the sampler's conversion
helpers. This is now a deliberate divergence from the main driver, which carries the same
latent defect on the line above its own guard and should take the same reordering.
FOUR CONJUNCTS WITH NO REAL COVERAGE. The audit deleted each of these and all 81 tests
still passed:
* lnL_offset=manual_avoid_overflow_logarithm at both call sites -- never driven at a
non-zero value, so its loss was invisible. Now tested at 1000.0, plus a source check
that both call sites pass it.
* the opts.sampler_method conjunct -- every test used sampler_method='AV'.
* the hasattr(bootstrap_from_samples) conjunct -- the test asserted the RETURN VALUE,
which is unchanged either way because the rescue's own `except Exception` swallows the
resulting AttributeError. It was measuring the exception handler, not the guard.
* the retry_neff conjunct -- with retry_neff=None, float(None or 0) is 0.0 and the
comparison was already False for that test's inputs.
Declining is now asserted by OBSERVABLE behaviour -- no "[L0 auto-rescue]" output and no
bootstrap -- via a shared _assert_declined helper, and each conjunct has a case where it
is the only thing declining.
THE LEDGER WAS NOT VERIFIED AGAINST ITS GENERATOR. The audit added an option to the main
driver and hand-wrote an entry into lisa_drift_ledger.json: all seven gate tests passed
while make_lisa_drift_ledger.py still reported the item as matching no rule. The stated
property -- that new drift must be classified AS A RULE, with a reason -- was silenceable
by a one-line JSON edit. A new test regenerates in memory and compares.
TWO STATEMENTS THAT WERE NOT TRUE OF THIS TREE.
* The ledger claimed _rvs_is_pooled was ported "as the reset-on-entry discipline plus
the reader". Only the reader was ported; nothing here ever sets the marker. Corrected,
and the ATTR category's blind spot (a name READ counts as present) is now recorded with
it.
* Three docstrings asserted behaviour of --sampler-sequential-warmstart, which this
driver does not have. The reserve restore they justify is still correct; it is
pre-emptive rather than load-bearing, and now says so. These are precisely the
docstrings the AST anti-drift test excludes, so nothing covered them.
Also documents what the drift audit CANNOT see -- it is a name-presence set difference, so
changed defaults, changed bodies, missing if-branches and runtime-built option names all
produce zero gap items. The audit defeated the gate with four such drifts; the honest
answer is that most are out of scope by construction, and the doc should not have implied
otherwise.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found by adversarial audit, NOT by the drift gate. The main driver has:
if opts.sampler_method =="AV" and opts.internal_use_lnL:
return_lnL=True
pinned_params.update({"use_lnL":True})
with the comment "without this, --internal-use-lnL --sampler-method AV passed the
ok_lnL_methods check but silently did nothing, so exp(lnL) overflowed at high SNR when no
logarithm offset was set." The LISA driver had branches for GMM, adaptive_cartesian_gpu
and portfolio -- and none for AV.
High SNR is the LISA MBHB regime, so this is the case rather than an edge, and it matters
more now that the preceding passes push AV and portfolio into LISA production.
This is a PRE-EXISTING defect, not one the catch-up introduced; the catch-up is what made
it worth finding. It is also a behaviour change for anyone already running
--sampler-method AV --internal-use-lnL on this driver: they were silently getting the
linear-integrand path, and will now get the log-space one the option asks for. Kept as its
own commit and its own PR so it can be reviewed or dropped independently of the ports.
WHY THE DRIFT AUDIT COULD NOT SEE IT. A missing `if` branch is not a FUNC, OPTION, CONST
or ATTR, so it produces zero gap items. The audit is a name-presence set difference:
behaviour behind a shared name is invisible to it. That limitation is now documented in
LISA_DRIVER_DRIFT.md, and test_lisa_use_lnL_branches.py closes this particular hole by
extracting the per-sampler pinned_params branch TABLE from both drivers and comparing them.
The table comparison immediately earned itself: it shows the portfolio branch still differs
by exactly the three --internal-gmm-* forwards (gmm_adaptive, gmm_defensive_frac,
gmm_inflate), which are the deferred GMM pass. That delta is asserted EXACTLY rather than
skipped, so any other divergence in that branch still fails and the test tightens by itself
when the GMM pass lands.
Revert-checked: removing the AV branch fails test_AV_sets_use_lnL_under_internal_use_lnL
and test_branch_table_matches_the_main_driver[AV]; file restored byte-identical.
Two related items the audit raised and this does NOT fix, both reported rather than
silently patched:
* bin/..._lisa lines ~2423 and ~3145 read sampler._rvs["integrand"] UNGUARDED (the
neighbouring argmax reads are guarded). Under AV that key can be absent, so
--maximize-only would KeyError. What those lines should print instead is a judgement
call, not a mechanical port.
* sampler.ntotal is not carried by _snapshot_pass_state, so a rejected warm pass reports
cold lnZ beside the warm pass's ntotal. Identical in the main driver, so it is a shared
defect rather than drift.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same false-green as the zero-collection bug fixed earlier in this branch, in a different disguise. The no-GPU path in the GPU consistency tests printed a line and `return`ed. That is right for script mode, but under pytest a test that returns without asserting is reported as PASSED -- so a CI run on a machine with no GPU would show green for the GPU parity checks having verified nothing. New _gpu_test_support.skip_without_gpu() raises a real pytest skip when running under pytest and falls back to the printed message when the file is run as a script, so both modes stay honest. Applied to test_q_window_interp_gpu, test_noloop_gpu_stencils, test_slowrot_gpu and test_slowrot_freqresponse_gpu. Verified both directions: no GPU (citlogin6) 8 skipped [was: 8 passed] with GPU (2080 Ti) 11 passed (includes test_calmarg_stencil_gating) test_calmarg_stencil_gating is deliberately NOT changed: it runs its CPU arms without a GPU and only adds a GPU arm when one is present, so it has no silently-empty path. The GPU re-run was checksum-gated against the local worktree before launching -- shared NFS $HOME is not instantly coherent, and a stale read gives a false green indistinguishable from a real pass. Also confirms the published CPU cost ratio: re-measured min-of-5 on an idle host, sinc/cubic = 4.15-4.50x across four configurations, matching the 4.2-4.5x already documented. An earlier 6.09x reading came from a host at load 26 and was contention, not signal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_audit_gpu_probe.py, _audit_gpu_probe2.py and _audit_edge_probe.py were throwaway probes written into the repo root during a review of this branch. They were swept into 959bd65 by a concurrent 'git add -A' in this shared checkout; they are not part of the change and belong nowhere in the tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A `git add -A` in this shared checkout captured a 757-line analysis script that belongs to concurrent work, is still being written to, and was never reviewed as part of this PR. Untracked (left on disk, so the work in progress is undisturbed). If any of it should ship, it should arrive as a deliberate, reviewed change. The same `git add -A` also captured three audit scratch scripts, removed in 69532bb. Both are the same mistake: staging by wildcard in a checkout several agents share. Stage explicit paths here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adversarial audit finding, and a real bug in the selection logic. --srate-internal overrides deltaT inside ILE (integrate_likelihood_extrinsic_ batchmode: `deltaT = deltaT_internal`), so it -- not --srate -- is the grid the sub-sample stencil steps along. It is appended to the ILE command line by util_RIFT_pseudo_pipe.py and never passes through the helper, which contains zero references to it. Result: --srate-internal 32768 with srate 4096, fmax 1700 made the helper see fNyq/fmax = 1.20 and pick 'sinc', while the run was really at 9.64, where this branch's own measurements put cubic 10-30x ahead. --srate-internal >= 4x the data rate is a documented requirement for low-mass runs, so this is live configuration space, not a corner. Second route to the same defect: the helper emits --srate only under --propose-ile-convergence-options, and ILE's own --srate default is 16384, not 4096 -- so on the branch where the CPU threshold is reachable the decision input was off by 4x by construction. Fixed with effective_srate_for_stencil(); pseudo_pipe forwards the internal rate as an explicit decision input (NOT a second --srate-internal emission). The test asserts the naive and corrected paths choose DIFFERENT stencils for that case, so the guard cannot decay into a no-op, and reads ILE's --srate default back out of the driver source so the duplicated constant cannot rot silently. Also from the audit: * The helper accepted an unvalidated stencil name and concatenated it onto every generated ILE command line; a typo built and submitted a whole workflow, then killed each job separately at run time. Validated at build time now. * The legacy scalar path was handed opts.interpolate_time raw. Once stencil NAMES became legal, '--interpolate-time nearest' would have switched that path's interpolation ON while meaning the opposite in NoLoop. It gets a derived boolean. * Both dispatchers ended in an unguarded `return cubic`, reinstating the silent wrong-stencil behaviour this work removes for anyone calling them directly. They raise. * Two comments in _with_rotation and _freqresponse still claimed 'sinc' is rejected on GPU. Untrue since Q_inner_sinc landed. * cupy's astype copies even when the dtype already matches, so the sinc wrapper allocated a redundant (n_ex, 2a) float64 buffer -- ~20 MB transient per detector per call at n_chunk=1.6e5, on the resource that caps n_chunk. * test_calmarg_stencil_gating runs its CPU arms without a GPU, so it joins the CI job; the commit that added it claimed CI coverage it did not have. And one the audit did not raise, found while fixing the above: the flag now takes a VALUE, so '--internal-ile-interpolate-time False' passes the STRING 'False', which is truthy in Python -- every "off" spelling would have sailed past the `if opts...:` guard and then been rejected as an unknown stencil. Added is_off_request(); the test asserts off / auto / stencil-name are disjoint. Audit also confirmed, by direct measurement, that this branch does NOT perturb existing results: the full NoLoop likelihood is bitwise identical to d904e72 for 'nearest' and 'cubic', CPU and GPU, n_cal=1 and n_cal=4, including cal_method='fused'. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follow-up to ff5b47f. The comment above the block still framed `srate` as the decision input and reassured the reader it was final. It IS final, but it is no longer what the choice is made from -- effective_srate_for_stencil is. Point at that instead, so nobody re-derives the bug from the comment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third adversarial round. The equivalence claim held -- 420 evaluated ad combinations found no case where the new request_memory disagrees with the old except the 20 where the old returned Undefined, and periodic_release is byte-identical across 13 configurations -- but four things needed fixing. `oom_retry_counter` is interpolated into arithmetic as well as into a comparison, and only the comparison is safe by precedence. A compound counter reassociated: `NumHolds - NumJobStarts` produced `int(1.5 * NumHolds - NumJobStarts * MemoryUsage)`, which for NumHolds=6, NumJobStarts=2, MemoryUsage=1000 asks for -1991 MB. condor accepts that and a negative request matches no slot -- the wedged-Idle failure the MemoryUsage guard was added to prevent, back through another door. The counter is now parenthesised in the bump; the release arm needs nothing, which is why the default text is still byte-identical. The comment block justifying #136's NumHolds swap was still sitting on top of the reverted code, telling the next reader that the counter is NumHolds and that this is deliberate, 1600 lines from a default that says NumJobStarts. Removed, along with the OUT_OF_MEMORY / MEMORY_LIMIT_EXCEEDED labels, which were both stale and swapped. The three new arguments were in neither the class docstring nor DESIGN.md, and the prose that WAS added stated the codes as fact ("hold codes 26 and 34 belong to the OOM policy") -- exactly what the change exists to make configurable. Both now describe the mechanism, and DESIGN.md points at `condor_config_val -dump | grep SYSTEM_PERIODIC_HOLD` rather than naming any site. Protecting periodic_release closed half a path. request_memory is the other half of the same policy, and replacing it leaves the release arm intact -- so the job is released the full oom_max_retries times at a fixed size and OOMs every time, spending the budget to no effect. It is protected too, and each refusal now names the option to use instead; the periodic_release message previously recommended the transfer knobs. Smaller, all from the same review: * A sub-code exclusion keyed on a code oom_hold_codes does not own is refused rather than ignored, so a typo cannot read as configured. This caught a nonsense fixture in the manifest test. * `q.oom_hold_codes = None` meant "own no codes" from the setter and "use the defaults" from the constructor. Now the latter in both. * The exclusions getter returns a MappingProxyType, so in-place mutation raises instead of silently doing nothing. * The `#:` block for _PROTECTED_SUBMIT_COMMANDS had been separated from it by the new constants; reordered. Removed test_the_policy_is_not_a_table_of_site_names. It was theatre: none of its needles occurred in the module even before the change, so it passed unconditionally and on the parent too, while the module does say "LIGO clusters" and "OSG access point" in prose it did not cover. A grep cannot express "no site-to-policy table"; the constraint is stated in DEFAULT_OOM_HOLD_CODES and DESIGN.md and enforced by review. 51 tests, 34 failing against rift_O4d. Suite clean under -W error::RuntimeWarning. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…y-schwarz slowrot: restore the arrival-time post-phase (lnL was exceeding 0.5<d|d>), NoLoop + scalar + JAX
test/jax: fix the 67.76-nat end-to-end 'mismatch' (harness fed the two paths different tvals grids)
…force-instrumentation test_slowrot_pathB_bruteforce: opt-in instrumentation (gwsignal/v5, sweep knobs, exact peak estimator)
…-floor claims (#152) * SLOWROT_HANDOFF: retract the catastrophic-cancellation and floor claims Documentation only. This file is what a developer picking up slow-rotation work reads first, and three of its claims are now known to be wrong. Two of them would actively send someone to do unnecessary work. 1. "the p>=3 catastrophic cancellation ... only bites at x1000+ inflation (>2.6x faster than any physical signal)". There is no catastrophic cancellation. It was the bug fixed in PR #117: term2 dropped the arrival-time post-phase and term1 concealed it via <e^{inOmega.}h|d> == <h|e^{-inOmega.}d>, an identity that is false for the NOISE-WEIGHTED overlap since a frequency shift does not commute with 1/S(f). Re-measured after the fix, the bound is respected at every rate from 0.5x to 3x and p=3 IMPROVES on p=2 at 1.5x/2x/3x by 9x/51x/5.7x. The 2.6x was also never measured: only x340 and x1000 were run, and 1000/340 = 2.94. That number propagated into the methods-paper draft and took a full investigation to remove. 2. "~0.1-0.2 resolution floor from NoLoop nearest-neighbour time sampling". That floor was a TRUNCATION artefact -- a 48.5 s chirp in a 16 s segment, so the delayed lookup ran off the array end and nan_to_num deleted the loudest samples. It survives a 4x finer time grid AND survives switching to cubic interpolation, so it was never the time lookup. 3. The open item "FIX the p>=3 high-frequency derivative blow-up by band-limiting the delay-derivative terms" is now marked MOOT. Implementing it would be work against a phenomenon that does not exist. The stale root-cause paragraph is kept, clearly marked superseded, as a record of what was believed. Post-fix numbers are inlined so the next reader does not have to re-derive them, and the physical-rate residual is stated as what it is: an upper limit at the test's own noise floor (1.7e-4 vs floor 1.5e-4), fractional agreement 1.6e-7. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * SLOWROT_HANDOFF: rest the floor retraction on the bug-immune INFL=1 evidence Self-review of the previous commit caught two defects in my own text. 1. 'Give the waveform a segment it fits in and it drops to ~1.6e-4' conflated two separate fixes. A fitting segment ALONE, pre-#117, still gave 0.053 at the physical rate; 1.7e-4 needed the segment AND the #117 post-phase fix. The 1.5e-4 figure is the ROTATION-OFF floor, which is a different quantity. Now stated explicitly, with the distinction spelled out. 2. The srate and cubic scans cited as support were run at INFL=340 with rotation ON, so their absolute numbers contain the #117 bug I was retracting elsewhere in the same file. The retraction now rests on the INFL=1 comparison, which is BUG-IMMUNE because the #117 error scales with Omega and vanishes at Omega=0 -- and which agrees to 1.5e-4 measured both before and after the fix, as it must. The contaminated scans are kept but explicitly fenced: qualitative trend only, do not quote the absolute values. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…cients populate (#142) (#143) * slowrot: the precompute must carry every harmonic the response coefficients populate rotation_coefficients builds C_{(p,ntilde)} by convolving the antenna harmonics (|n|<=2) with the delay-drift harmonics (|m|<=1) once per derivative order, so the harmonic index widens by exactly one per order: the bank needs |ntilde| <= 2 + p_max. PrecomputeLikelihoodTermsWithRotation built one band per REQUESTED harmonic, and a coefficient with no band is dropped without complaint by both maintained evaluators (the NoLoop's Cg/Cg_d return zero for a missing a; the JAX packer in jax_ile.banded packs only a_list). The default harmonics=(-2..2) is the p_max=0 answer, so any direct caller running Path B with the default got a quietly truncated model. Measured (H1, p_max=1, requested (-2..2)). The complete p=1 block is a delay-drift correction whose fourteen terms cancel: lnL Path A = 1781.4905, complete Path B = 1781.4855, i.e. -0.005 nats, exactly as Path B must reduce to Path A for a short signal. Dropping the two terms C_{(1,+-3)} turns that 0.005-nat correction into a 7662-nat error (lnL = -5880.86). The dropped coefficients look negligible (0.48% of the largest kept |C|) but carry 191% of its effective band amplitude, because the p=1 bands' norm is larger by 1.6e5 = (2 pi * 63.8 Hz)^2. Both still respect 0.5<d|d>. Fix: the precompute widens `harmonics` itself to the union with -(2+p_max)..(2+p_max) and raises a RuntimeWarning naming the required width, so the widening is not itself silent. The rule now lives in one place, required_harmonic_width(); the ILE's existing max() guard is KEPT (its _harm is printed, and --rotation-n-harmonics is a user-facing floor) but now calls that function instead of open-coding 2 + p_max. meta records harmonics_requested / harmonics_required / harmonics_truncated, and pack_rotation_arrays -- the gateway from a bank to the NoLoop -- warns when handed a truncated one, so the escape hatch cannot be used silently either. widen_harmonics=False is that escape hatch, used at exactly one site: test_V0_recovers_baseline, which passes harmonics=(0,) to isolate the a=(0,0) band and never assembles a likelihood. New test_slowrot_harmonic_width.py measures the antenna/delay half-widths and the p_max=0..3 index sets (so the rule is measured, not asserted), asserts the widened bank drops no coefficient in the numpy, JAX and vectorized-NoLoop paths, and carries an in-tree control proving the guard can fail. Mutation-tested: reinstating the truncation fails W2-W6 while the existing suite -- including test_slowrot_pathB -- stays green, which is the gap this file closes. Closes #142. * slowrot: correct the jax gate's hard-coded truncated bank size; guard the JAX packer Two follow-ups after merging #117 + #144 into this branch. 1. test/jax/test_jax_slowrot.py asserted len(meta['a_list']) == (p_max + 1) * len(HARM) which with HARM=(-2..2) hard-codes 10 bands at p_max=1 -- the TRUNCATED width this PR is about. The jax suite therefore did not merely miss #142, it asserted it: the widened bank fails with "assert 14 == ((1 + 1) * 5)". The expectation now derives from required_harmonic_width(p_max), and the test also asserts that meta['harmonics'] really was widened. The file's docstring band/cross-term costs were stale for the same reason (15 bands / 225 cross terms at p_max=2, 100 at p_max=1); they are 27/729 and 14/196. Isolated with controls rather than guessed: unmodified 98becce passes 3/3 in the same interpreter, so this was not the float32 jax in RIFT_develUWM (my first hypothesis) and not a numerical regression. 2. jax_ile.banded.build_rotation_data now warns on a bank with meta['harmonics_truncated'], beside #117's post_phase_required check and in its shape. This gap was deferred in the previous commit only to avoid a conflict with #117 in that function; #117 has landed, so the reason is gone. W5 was extended to drive the real packer both ways (14 bands quiet / 10 bands warns) rather than only checking coefficient keys. Re-measured on the merged base, since #117 changes the likelihood: Path A 1781.4876, Path B complete 1781.4859 (-0.0017 nats -- the p=1 block still cancels), Path B truncated -5880.8801 (-7662.3677 nats). Mutation re-run: W2-W6 fail, W0/W1 survive.
… count Nothing in .github/workflows/ci.yml ran test/jax/ -- the file had zero matches for "jax". Two real defects survived behind that gap: the 67.76-nat test_jax_endtoend failure (broken 2026-07-15 by 3360ce1, found 2026-08-18, PR #144) and the jax_ile slow-rotation post-phase gap (#131/#132), which was found by reading code, not by a test run. Both are now fixed on rift_O4d; what is still missing is anything that RUNS the tests that guard them. The obvious repair -- point pytest at test/jax/ -- would have manufactured more confidence than it earned. Several files there are __main__ scripts with no test_* function; pytest collects ZERO items from those and exits 5, "no tests ran", which reads as a pass. Measured per file on a pristine 88959ef worktree: 11 tests collected across 9 files, with test_flow_reuse, test_jax_slowrot_wrapper, test_network_coords and test_nuts_phimarg each at exit 5, and test_nuts_phimarg_injection not collectible at all (its module body IS the study). * Thin pytest entry points added to test_jax_slowrot_wrapper.py (1), test_network_coords.py (1) and test_nuts_phimarg.py (1) -> 14. The nuts one asserts main() == 0: that file reports through its return code, so a bare main() call would have passed on a FAILED run. * test_jax_slowrot.py already had three entry points, but they called only the check_* halves; the file's AD/jit/vmap/hessian gates ran solely from __main__ and would have been collected-but-unexercised. They now run in the entry points, and __main__ calls the entry points, so the two paths cannot drift. * .travis/test-jax.sh runs eight files and asserts a floor of 14 collected tests BEFORE running anything, so a refactor that silently zeroes or thins collection turns the job red instead of green-on-nothing. Any nonzero pytest exit fails, exit 5 included. * jax-ile-check job, python 3.11 (current jax wheels need >=3.11), installing jax[cpu] + numpyro, JAX_PLATFORMS=cpu, no GPU, timeout-minutes 60. Excluded with reasons in the script: test_nuts_phimarg_injection.py (module-scope study, not collectible, >1800 s) and test_flow_reuse.py (passes in 302 s but needs an unpinned flowMC). Verified on ldas-pcdev11, JAX_PLATFORMS=cpu, OMP_NUM_THREADS=1, jax 0.9.2 / numpyro 0.21.0 / pytest 9.1.1. Exit codes observed, not inferred: clean run of `bash .travis/test-jax.sh` exit 0 14 passed in 907 s (964 s total) mutation A: 1e-11 -> 1e-30 in coeffs exit 1 1 failed, 13 passed mutation B: drop one test_* entry point exit 1 "collected 13, expected at least 14", 55 s The GitHub Actions job itself was NOT run -- Actions cannot be run from here. Only that the YAML parses was checked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…w findings) Review verdict was "sound gate"; these close the holes it found in the gate's own promise. VERIFICATION INCOMPLETE -- see the end. * OUTCOME check (finding 1, the important one). The floor counted COLLECTION and never outcomes, so a single pytest.skip()/importorskip() disabled a gate while both the collected count and the pytest exit status stayed green -- the exact shape this script exists to prevent. Now writes --junit-xml and requires tests>=EXPECTED_TESTS, skipped=0, failures=0, errors=0. * MANIFEST check (finding 2). FILES is a hand-maintained allowlist, so a newly added test/jax/test_*.py would have been silently ungated while the job stayed green -- this gate's own bug, one level up. Every test_*.py must now appear in FILES or in an explicit EXCLUDED array, or the job fails in seconds. * ANCHORED count (finding 3). grep -c '::' also matched merged stderr (jax/XLA log lines, C++ symbols, '::1'); under a >= floor OVER-counting is the dangerous direction, where one stray line masks exactly one lost test. Now '^<path>.py::'. * Exclusion rationale corrected (finding 5): test_nuts_phimarg_injection fails collection fast only WITHOUT numpyro; with numpyro -- which this job installs -- --collect-only executes the study and hangs, so re-adding it would burn to timeout-minutes. * One pip invocation (finding 7) in jax-ile-check, so pip co-resolves; installing jax separately let a numpy bump past numba's ceiling land as a warning with exit 0 and surface later as a collection error looking like a RIFT bug. * setup.py claim corrected (finding 8): extras_require['jax-apps'] is for interpolators.jax_gp and lists a different set; likelihood.jax_ile declares its dependencies nowhere, so this job is their de-facto declaration. * cd to repo root; noted why -e is deliberately absent (finding 9). * Recorded the OBSERVED runner result in ci.yml, replacing the guessed timeout rationale: job 95750869285, python 3.11.15, jax 0.10.2, numpyro 0.21.0, 14 passed in 285.96 s, 6m07s wall. The unpinned install had already drifted a jax minor version from the 0.9.2 measured locally and still passed. VERIFICATION STATUS: shell syntax (bash -n) and YAML parse both OK, and the full diff was reviewed. The mutation tests for the two NEW guards (manifest, outcome) were RUNNING when the whole interactive fleet -- pcdev11/12/13 and citlogin6 -- went down for maintenance, so they are NOT yet confirmed to fail as intended. Do not mark this PR ready until they have been re-run and observed failing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…und check The jax-ile-check gate caught this on its first run against a rift_O4d containing the harmonic-widening fix (#142/#143). CI builds the PR MERGE commit, so it was the first thing anywhere to run the JAX tests against the widened precompute; local runs of this branch alone were green and irrelevant. TRACK THE WIDENING. rotation_coefficients emits keys (p, n+m) with |m| <= 1, so the coefficient index widens by one per derivative order, and the precompute now widens a too-narrow `harmonics` to |n| <= 2 + p_max rather than dropping bands. This test derived its harmonic set from HARM in three places -- the a_list assertion, the reference model's band list, and the printed band count -- so data, bank and reference model would have sat on three DIFFERENT sets at p_max >= 1. All three now come from flwr.widen_harmonics_for_p_max, the same helper the precompute uses, so they cannot drift. A=14 bands at p_max=1, as the widened set requires. SCOPE (A) AND (B) TO p_max=0. On the widened bank the p_max=1 rung reads: (A) static deficit 0.3907 nats (threshold 1.0) (B) bound overshoot -4.108e-03 nats -- VIOLATED (C) vs explicit model 3.091e-02 nats = 6.06e-07 relative -- passes (D) vs numpy NoLoop 2.488e-09 nats -- passes (B) is NOT the evaluator's: the numpy NoLoop overshoots by the same -4.108e-03, the two agreeing to 2.5e-09. The overshoot is in the data/reference construction, whose own conditioning (C) measures at 6.06e-07 relative, ~0.03 nats -- 8x larger than the 0.004 nats the bound is trying to resolve. At INFL=1350 with fmax=1700 the delay drift gives 2*pi*f*delta_tau ~ 85, so the p-expansion is divergent at the top of the band. (A) is a physical fact, not a defect: with the non-truncated model the static approximation really is good to 0.39 nats at this rate. So asserting either at p_max=1 would mean loosening a tolerance to fit numerical noise, or asserting something false. Both are scoped to p_max=0, where the rung is exact (deficit +0.000000, (C) 1.28e-15), with all four measured numbers recorded inline and an explicit instruction not to widen TOL_BOUND. (C) and (D) still run at p_max=1 and are what pin the evaluator there. Restoring the bound at p_max=1 needs a converged configuration: issue #159. Note the pre-#142 numbers for this rung ((A) 36.4, (B) +5.076e-04) looked healthier only because the bank was missing its |n|=3 bands, i.e. they were measured against the wrong signal. Not a regression target. Gate after this change: manifest check ran, collected 14 from 8 files, 14 passed in 335.36 s, junit tests=14 skipped=0 failures=0 errors=0, PASS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ci: run test/jax/ (jax_ile), with a pinned floor on the collected test count
…release simulation_manager: make the OOM hold policy an argument, not a constant
bin/integrate_likelihood_extrinsic_batchmode built
linspace(-t_ref_wind, t_ref_wind, int(2*t_ref_wind/deltaT)) at ten sites;
RIFT/likelihood/jax_ile/wrapper.py -- and so
bin/integrate_likelihood_extrinsic_jax -- built arange(-Nw, Nw)*deltaT with
Nw = int(iwh/deltaT). Both likelihoods consume only tvals[0] and len(tvals):
each steps by deltaT and integrates with dx=deltaT regardless of the grid's own
spacing. So the grids differed in ORIGIN by 0.2 samples, enough to round
ifirst = rint((t_det + tvals[0])/deltaT) + 0.5 to a different integer sample --
and, since t_det carries the per-detector delay, a different subset per
detector. They also differed in LENGTH at srate 1024, 2048 and 16384, because
2*int(x) != int(2*x); 16384 is the low-mass production rate.
Adds factored_likelihood.marginalization_time_grid(iwh, deltaT, xpy) as THE
constructor:
npts = int(2*iwh/deltaT) # batchmode's length, unchanged
tvals = (arange(npts) - npts//2)*deltaT # spacing exactly deltaT
arange, not linspace, because it is the only convention where tvals[k] labels
the time the code actually evaluates; linspace mislabelled its own samples by up
to 1.4 samples (3.4e-4 s) at the window edge. npts from int(2*iwh/deltaT), not
2*int(iwh/deltaT), so production window LENGTHS do not change at any rate; the
former JAX default gains one sample at 1024/2048/16384.
All ten batchmode sites and all three wrapper sites now call it. Also:
- the time-resampling export comment no longer claims the internal grid is a
linspace spaced "~4086.7 Hz vs 4096" -- it is now exactly deltaT-spaced, and
the tvals it reads as time LABELS for the exported t_ref are now the times the
likelihood evaluated;
- FactoredLogLikelihoodTimeMarginalized integrates with dx = tvals[1]-tvals[0],
which was 0.23% too large under linspace (the loop path steps by whole
samples); it is now exactly deltaT;
- test_jax_endtoend's grid pin and the jax_ile core/wrapper docstrings are
updated to the shared convention.
New test test/jax/test_tvals_grid_convention.py extracts every window-grid
construction from the driver sources BY AST -- recognising the two legacy
spellings as well as the helper, so it is not a helper-presence check -- and
compares them by value at srate 1024/2048/4096/8192/16384. Verified to FAIL on
unmodified rift_O4d at all five rates, including 4096 and 8192 where the old
lengths coincidentally agreed. Registered in .travis/test-jax.sh
(EXPECTED_TESTS 14 -> 26).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three findings from reviewing my own PR before un-drafting.
1. test_all_driver_grid_sites_agree_by_value could pass VACUOUSLY. It used
sites[0] as the reference and compared the rest against it, so if either
file contributed zero sites it would compare one file against itself and
pass. That is the single-path-conjunct shape this file exists to avoid.
It now asserts both files contributed, inside that test rather than only
in test_extractor_actually_finds_the_sites, so the test cannot pass
vacuously on its own. Verified: replacing the wrapper's three helper
calls with an unrecognised constructor now fails the value test at all
five rates ("got 10 from batchmode and 0 from the wrapper"), where before
it would have gone green.
2. Nothing pinned that bin/integrate_likelihood_extrinsic_jax actually
INHERITS the wrapper default. The cross-driver test compares batchmode
against jax_ile/wrapper.py, which is only a valid proxy for "the two
drivers agree" while that driver passes no tvals= of its own. New test
test_jax_driver_takes_the_wrapper_default_grid asserts it. Verified by
adding a tvals= argument to that driver: the new test fails, and nothing
else did.
3. CHANGES.rst had no entry. This changes production lnL values, so it needs
one users can find when a rerun disagrees with an archived result.
Also verified while reviewing, no code change needed:
- batchmode's window LENGTH is unchanged over all 110 (iwh, srate) pairs
tested, not just the five in the test. The npts expression is
byte-for-byte batchmode's own.
- scipy's Simpson weights differ structurally between even and odd npts
(endpoints 0.4167/1.0833 vs 0.3333/1.3333), and jax_ile/core.py builds
its quadrature with _simpson_weights = simpson(eye(npts)). So the JAX
side's 2456 -> 2457 change DOES switch quadrature branch -- and the
jaxside measurement already exercised it through the same simps, at
max|d| = 2.2e-4 nats. Odd npts is the exact Simpson case; the old even
length was the fudged one.
- no window-grid construction was missed in batchmode.
- FactoredLogLikelihoodTimeMarginalized is the ONLY function in
factored_likelihood.py using a grid-derived dx (AST-checked); it is
reached from two of batchmode's three numpy sites, not three as the PR
body said. Corrected there.
EXPECTED_TESTS 26 -> 27.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tvals: one window-grid constructor for both ILE drivers (#146)
Give the retained set and the export resample separate names
… p_max=1 CS rung Closes the p_max=1 Cauchy-Schwarz rung opened in #159. The rung's (A) and (B) checks were scoped to p_max=0 in #151 on the diagnosis that the delay Taylor series had diverged. That diagnosis was wrong, and the sweep says so: dropping fmax from 1700 to 64, which takes max|2 pi f delta_tau| from 30.4 to 1.1, does not move (C) at all. fmax (INFL=1350, p_max=1) 1700 1024 512 256 128 64 (B) overshoot [nats] 4.11e-3 5.72e-3 6.56e-3 7.06e-3 7.27e-3 6.86e-3 (C) relative 6.06e-7 5.56e-7 5.35e-7 4.88e-7 5.37e-7 1.18e-6 Two separate defects were responsible. 1. THE NYQUIST BIN of time_derivative_weight, FOR ODD p (a library defect, shared by the numpy NoLoop). The RIFT two-sided packing carries +fNyq but not -fNyq, so that one bin serves both signs; a weight can only do that when it is EVEN in f, i.e. when p is even. For odd p, conj(h^(p)) and (conj h)^(p) -- the same function -- differed there by a sign. U takes both factors from the same template family and never noticed; V = <chi_a^*|chi_a'> pairs the two orders and did. The sidereal modulation is a sub-bin shift applied as a time-domain phase, so it spread that one bin across the whole band. |H(+fNyq)| is 0.02-0.14 of |H(100 Hz)| for these modes, so this was worth 1.5e-07 of the p_max=1 model norm -- a norm too SMALL, which is how lnL got 4e-03 nats OVER the bound. EVEN p IS LEFT ALONE, deliberately. (2 pi i fNyq)^p is real for even p, so there is no ambiguity, and the derivative IS representable: d^2/dt^2 (-1)^j = -(2 pi fNyq)^2 (-1)^j. An earlier revision of this patch zeroed every p >= 1; against the analytic derivative of a Nyquist-carrying multitone that was 90% relative error at p = 2 and 99% at p = 4 (the untouched weight is exact there to 3e-14), and moved a real p_max=2 bank by 0.207 nats -- larger than the defect being fixed, and reachable from --rotation-p-max 2. p_max=1 output is bit-identical either way, so the even-p half bought nothing. The zeroing fires only when the extreme-|f| bin is genuinely UNPAIRED. Testing magnitude alone would blank BOTH ends of a symmetric axis, where nothing is wrong; no RIFT packing is symmetric, but the predicate should say what it means. Localisation, at INFL=1350 / fmax=512 / p_max=1, arrival offset k=0 so the post-phase is the identity: the data term <d|h> already agreed to 1.0e-15, only <h|h> was off (1.46e-07); the evaluator reproduced the bank's own U/V to 1.4e-16; hlms_conj matched the exact conjugate spectrum to 1e-15; swapping V alone for an independently built family moved <h|h> from -1.4925e-02 to -1.3e-10. 2. THE SHIFT CONVENTION of (C)'s own reference. The bank shifts the MODULATED elementary template circularly and repairs the phase with rotation_post_phase; the reference modulated on the unrolled grid. Analytically identical, but e^{i n Omega u} is not periodic on the segment, so the two differ by e^{i n Omega T_seg} on the K_ARR samples that wrap. hY^(0) is machine zero there (1.2e-16 of its peak) and hY^(1) is not (5.9e-04), so p_max=0 never saw it. With defect 1 fixed but the reference left unrolled, (C) reads 3.26e-07 relative at INFL=1350 and 2.55e-06 at the INFL=5400 this rung ships. With both fixed, (A) and (B) are asserted at p_max=0 AND p_max=1: p_max=0 p_max=1 (A) static deficit 4.9865 nats 3.9234 nats gate > 1.0 (B) bound deficit +0.000000e+00 +5.602e-10 gate overshoot <= 1e-6 (C) vs explicit 5.821e-11 = 1.14e-15 rel 6.476e-10 = 1.27e-14 rel (was 6.06e-07) (D) vs NoLoop 5.821e-11 8.222e-10 No tolerance was widened. Path B now runs at Omega*T_segment for a 6-hour signal rather than 90 minutes: (A) scales sub-quadratically in Omega (0.0046 / 0.107 / 0.389 / 1.296 / 3.923 nats at INFL = 135 / 675 / 1350 / 2700 / 5400) and the 90-minute rate left it at 0.39, below the gate. (B) and (C) are at machine precision across that whole range once the two defects are fixed, so the rate is free to choose. (INFL, fmax) are per-rung via a small Config class. Two things the rung now says explicitly that it does NOT claim: that the p-expansion converges here (it does not -- max|2 pi f delta_tau| = 184.9, and that is fine because the data is the exact model at the p_max under test), and that the bank's circularly shifted model matches a physically modulated one (it differs by 0.130 nats = 2.55e-06 relative at this configuration, a property of FFT-correlation banks that a Path-B production run inherits, and no assert here covers it). Guards, each mutation tested against every wrong weight it is supposed to catch: test_derivative_commutes_with_conjugation_at_nyquist consistency test_nyquist_derivative_value_both_parities the VALUE, at both parities weight variant commutation value pre-existing test_time_derivative_exact shipped PASS PASS PASS old (unfixed) FAIL(1.69) FAIL PASS zeroed all p PASS FAIL PASS w[fNyq] real + PASS FAIL PASS w[fNyq] real - PASS FAIL PASS zeroed even p FAIL FAIL PASS Consistency alone does NOT pin the weight -- any real w[+fNyq] commutes with conjugation and keeps a real series real -- which is why the value test exists. Both run p = 1..6, since --rotation-p-max is an unbounded int. The commutation gate is 1e-9, the same one test_time_derivative_exact already uses: it is a ROUNDOFF bound (measured 2.8e-15 to 3.3e-11 across p = 1..6 with the fix in, the residual growing with p as (2 pi f)^p amplifies the FFT round trip), and the defect it catches is 1.7e+00 to 3.0e+01 -- eight orders clear. The value gate stays at 1e-12 and reads exactly 0. Post-phase mutation, jax_ile/core.py, BOTH rungs: * from both terms: (B) stays silent (0.057 / 0.993 nats UNDER the bound) and (C) fires at 95.31 nats = 1.87e-03 rel (p_max=0) and 231.33 nats = 4.54e-03 rel (p_max=1). * from the model norm only: (B) fires, 10.57 nats (p_max=0) and 16.75 nats (p_max=1) OVER. * (A) does not move under either, correctly: it compares the evaluator against ITSELF at f_sidereal=0, so it guards the configuration, not the post-phase. Documented inline. The same defect class exists unfixed in the Path-D twin (finite_size_response_weights is 99% imaginary at +fNyq for W_1/W_2/W_4, 17% for W_3/W_5, while its docstring claims Hermiticity); it is latent there because Path D has no modulation to spread the bin. Filed as #164 rather than widened into this PR. Verified on ldas-pcdev11, CPU, float64: .travis/test-jax.sh 14 passed, skipped=0, failures=0 (875 s) numpy slowrot suite 37 passed (11 files, 185 s; 35 on the base commit + 2 new) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ax1-conditioning slowrot: fix the Nyquist bin of the FD derivative weight; restore the p_max=1 Cauchy-Schwarz rung
…yquist bin Closes #164, the twin of the #159/#163 defect, found by sweeping for it rather than by hitting it. finite_size_response_weights documents "Each W_p is Hermitian (W_p(-f)=conj(W_p(f))) so the V cross term needs NO harmonic reflection", and factored_likelihood_freqresponse builds on exactly that: etac = W_p * conj(h_lm) paired with eta = W_p' * h_l'm' to form crossTermsV_fr = <conj(W_p h) | W_p' h'>. That identification needs conj(W_p h) == W_p conj(h) bin by bin. The claim holds everywhere it can and fails where it cannot. RIFT's packing carries +fNyq at k=0 but no -fNyq (the bin holding -f[k] is npts-k, which for k=0 is bin 0 itself), so that one bin stands for both signs and Hermiticity there means REAL. Measured at npts = 16384, deltaF = 0.25 (f[0] = +2048 Hz): Hermiticity at every PAIRED bin is exactly 0.00e+00 for every p, while at the unpaired bin |Im W_p| / |W_p| is p 1 2 3 4 5 L = 4 km 0.9935 0.9853 0.1708 0.9853 0.1708 (W_0 = 1 is already real) L = 10 km 0.9596 0.9093 0.4162 0.9093 0.4162 L = 40 km 0.4655 0.1456 0.9893 0.1456 0.9893 (CE: 47% of |W| at that bin) Fix: project that one bin onto its real part, which IS the Hermitian average (W_p(+fNyq) + W_p(-fNyq))/2 -- the response the grid's only Nyquist degree of freedom, the real alternating sequence (-1)^j, actually sees. Same resolution as #159; there the Hermitian average happens to be zero for odd p and the untouched value for even p, which is why that fix is parity-dependent and this one is not. The predicate tests UNPAIREDNESS, not magnitude: a one-sided analysis band's top bin is not a Nyquist bin and must not be touched, and a symmetric axis carrying both +/-fmax has no unpaired bin at all. THIS MOVES NO NUMBER, and the reason is sharper than "the bin is out of band". lalsimutils.ComplexIP fills its one-sided weights with range(minIdx, maxIdx) -- HALF-OPEN -- so the fMax bin gets weight zero; at fMax = fNyq that bin is +fNyq itself. The bin therefore carries weight exactly 0 in every RIFT overlap at every fMax. Verified by stressing rather than by reading: scaling the bin by 1e6 in all W_p changes crossTerms_fr, crossTermsV_fr and rholms_fr by exactly 0.000e+00, at fMax = 1700 AND at fMax = fNyq = 2048. A direct raw-vs-fixed precompute diff is likewise 0.000e+00 at fMax = 1700 / 2000 / 2048. So this repairs the primitive and its stated contract, not a wrong result. What made #159 severe was not the bin's weight but a mechanism to MOVE it -- the sidereal modulation is a sub-bin shift applied as a time-domain phase, and the FFT round trip smeared the bad bin into bins that do carry weight. Path D has no such step today; anything later that mixes frequencies, or any consumer that indexes W directly rather than going through ComplexIP, would make it live. Five guards in test_slowrot_freqresponse.py, each run over SEVEN (arm length, Qmax, npts) combinations -- (4 km, 0/1/4/6), (10 km, 4), (40 km, 2/6), npts 4096..32768. That parametrisation is not decoration: --freqresponse-arm-length and --freqresponse-qmax are both user-settable, the defect's size at the unpaired bin depends strongly on L (table above), and with the guards pinned at a single (4 km, Qmax=4, npts=16384) point THREE wrong builders passed all five -- ones that project correctly there and silently decline for a 40-km CE arm, for another Qmax, or for a larger grid. Found by internal adversarial review. Mutation table (eleven wrong builders; the projection-scope column is the new one): builder variant predicate hermitian commutation value scope shipped PASS PASS PASS PASS PASS unprojected (pre-fix) PASS FAIL FAIL FAIL FAIL bin set to 0 PASS PASS PASS FAIL FAIL bin set to |W| PASS PASS PASS FAIL FAIL bin set to Im W PASS PASS PASS FAIL FAIL only W_0 projected PASS FAIL FAIL FAIL FAIL EVERY bin projected PASS PASS PASS PASS FAIL one-sided top bin too PASS PASS PASS PASS FAIL declines for a 40-km arm PASS FAIL PASS FAIL FAIL declines if Qmax != 4 PASS FAIL PASS FAIL FAIL declines if npts > 16384 PASS FAIL PASS FAIL FAIL Rows 7-8 are why the scope test exists: a builder that takes the real part of every bin destroys the whole response phase, yet is trivially Hermitian and its unpaired bin is trivially its own real part -- consistency AND value both pass it. Only asserting what the projection is NOT allowed to touch catches it. Same lesson as #163, where a consistency-only guard admitted three wrong weights. Two mutants still pass all five, and both are acceptable rather than holes: one returns a read-only array (every value correct; only writeability differs, and no caller writes), and one swaps in time_derivative_weight's guard verbatim, which differs from this one only on an all-negative axis that no caller can produce. Also, from the same review: * time_derivative_weight now names this function. The previous message claimed the two "name each other" and shared no imports; neither was true -- the reference existed in one direction only, and both modules import numpy. Corrected in both docstrings, along with the fact that the two guards are NOT byte-identical (that one declines on `not any(f<0)`, this one on `not (any(f<0) and any(f>0))`). * _continuum_weights in the test file is now documented as what it is: a hand copy of the production formula, and the only thing in the file that would notice the FORMULA moving (flipping the delay-phase sign passes every other check). The scope test's failure message now tells those two cases apart instead of always blaming the projection. * finite_size_response_weights documents that its value at the extreme bin depends on the AXIS, not on the frequency alone -- slicing fvals[fvals>0] returns the unprojected value there, a 17% difference at 4 km. Two findings are left unfixed and filed instead: antenna_response_fd / F_fd_expanded carry the same non-Hermiticity at the same bin (#168 -- they are continuum functions, not grid objects, so the right fix is at their grid-building call sites), and the whole numpy slowrot suite, including these guards and #163's, runs in CI nowhere (#169). Verified on ldas-pcdev11, CPU, float64: .travis/test-jax.sh 27 passed, skipped=0, failures=0 (847 s) numpy slowrot suite 42 passed (11 files, 174 s; the 5 new tests are parametrised in-body, so the collected count is unchanged from the first revision of this branch) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…esponse-nyquist slowrot Path D: make the response weights Hermitian at the unpaired Nyquist bin
…ce to the paper repo Records-protocol cleanup on my own merge (#165). Docstrings only -- verified mechanically, not by eye: parsing both revisions and comparing ASTs with all docstrings stripped gives an identical tree, and all 17 compiled code objects match on co_code, co_names, co_varnames and every non-string constant. The two docstrings had grown into a report: 55 and 20 lines carrying measured |Im W_p|/|W_p| tables, a reading of ComplexIP's source, a 1e6 stress-test result, and a cross-reference narrative about #159. That is evidence -- expected to be superseded, imported by nothing, and read as authoritative by everyone -- so it is promoted to RIFT_roboto_paper analyses/slowrot_nyquist_bin/NOTE.md, which says up front that it is a record and that the code's tested constants win on disagreement. What the code keeps is the durable shape: the API, the constraints the source cannot show, the anti-instructions at the site they guard, and a pointer. * W_p is Hermitian, which is what lets the V cross term skip a harmonic reflection. * The unpaired extreme bin must be real, or crossTermsV_fr is not the term it claims. * This is a GRID object, not a pointwise map f -> W(f): the extreme-bin value depends on the axis, so build the weights on the axis the overlap will use. * unpaired_extreme_bin tests unpairedness, not magnitude, and is not interchangeable with the Path-B guard. * DO NOT REMOVE THE PROJECTION ON THE GROUNDS THAT IT CHANGES NOTHING -- it is a no-op only because ComplexIP gives that bin zero weight, and any later step that mixes frequencies, or any consumer indexing W directly, makes it live. That last one was DELETED by the first revision of this patch and caught by adversarial review before un-drafting. It was the worst possible thing to drop here: the note argues in bold that the projection moves no number, the guards pinning it run in no CI (#169), so the docstring was the only standing defence against someone deleting a proven no-op and its tests. Restored to the code; the note now qualifies "moves no number" as contingent and names what makes it live. The 17% axis-slicing figure, dropped rather than routed by the same revision, is now recorded in the note. finite_size_response_weights 55 -> 27 docstring lines unpaired_extreme_bin 20 -> 13 Pointers name the immutable issue/PR first and the paper path second, so they stay meaningful under either merge order. Verified on ldas-pcdev11: test_slowrot_freqresponse.py + test_slowrot_freqresponse_likelihood.py 12 passed; full gate 27 passed jax / 42 passed numpy; and the eight-builder guard mutation matrix in mutate_fr_distill.py reproduces the pre-distillation tree row for row (only `shipped` passes every guard; the other seven each fail at least one). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ds-cleanup slowrot_freqresponse: distil the Nyquist docstrings; route the evidence to the paper repo
…g a rate Three changes, all closing gaps the #163 review found. 1. THE NYQUIST PARITY GUARD RAN IN NO CI JOB. RIFT/likelihood/test_slowrot_fd_ops.py is the only automated check on the parity of time_derivative_weight's Nyquist handling -- that bin must be zeroed for ODD p and left alone for EVEN p. It appeared in no workflow. The jax gate cannot substitute: at p_max=1, which is what test_jax_slowrot_cauchy_schwarz exercises, the correct weight and the over-zeroing revision #163 rejected are BIT-IDENTICAL, because p=1 is odd either way. Re-landing that revision was a green CI run. Added to q-window-stencil-check (numpy + lal only, a few seconds). 2. config_for() GUESSED A RATE FOR AN UNLISTED p_max. It fell back to the Path-A default, so run_ladder(p_max=2) -- which the module docstring invited -- silently ran at a rate the file's own asserts reject, and failed them. It now RAISES, and the invitation is withdrawn rather than left pointing at a path that fails. p_max=2 stays unsupported because (D), JAX against the numpy NoLoop, exceeds TOL_NOLOOP at every configuration tried and TOL_NOLOOP is absolute-only; supporting it means giving (D) the `abs OR rel` shape (C) already has, which is a change to what the test asserts. 3. TWO COVERAGE GAPS CLOSED WITH KNOWN-ANSWER TESTS. * test_nyquist_guard_clauses_on_synthetic_axes pins the guard branches of time_derivative_weight that no production axis reaches -- one-sided, symmetric, fftfreq-ordered and degenerate frequency axes. Four mutations of those branches previously survived the whole file; each now dies on the sub-case that targets it. (f.ndim < 1 -> f.ndim < 0 remains an EQUIVALENT mutant, documented as such so a future sweep does not chase it: a 0-d array always has size 1, so the size test covers it.) * test_rotation_post_phase_is_not_the_identity pins rotation_post_phase against a known answer. That helper is the documented convention, named by ~20 comments across the likelihood and the jax port, but it has one call site and production routes through the NoLoop's inline copy -- so neutering it to `return dict(C)` left BOTH the numpy suite and the Cauchy-Schwarz ladder green. The ladder built to guard exactly this fix could not see it. Also: the module's evidence prose is routed to its record store per the records-protocol skill. Measured tables, mutation numbers, sweep results and PR archaeology move to RIFT_roboto_paper analyses/slowrot_bound_violation/ (PR #19); the code keeps the decisions, the anti-instructions at the sites they guard, the constraints the source cannot show, and pointers that name the immutable PR first. Four docstrings, ~-300 lines. VERIFICATION (ldas-pcdev11/13, JAX_PLATFORMS=cpu, JAX_ENABLE_X64=1, OMP_NUM_THREADS=1): jax gate `.travis/test-jax.sh` 27 passed, junit tests=27 skipped=0 failures=0 errors=0 q-window-stencil-check 40 passed, exactly the file list ci.yml runs test_slowrot_fd_ops.py 9 passed mutation: re-land the over-zeroing revision 1 failed, rc=1 (was green before this PR) mutation: never zero the bin (pre-#163) 3 failed, rc=1 mutation: over-wide mask 0.9*fn 1 failed, rc=1 mutation: each of the four guard branches 1 failed, rc=1 each mutation: neuter rotation_post_phase 1 failed, rc=1 (was green before this PR) All mutation batteries run on `git archive` extracts with the import asserted to resolve into the sandbox, anchors asserted unique in Python, and a known-lethal control in every battery. NOTE FOR ANYONE READING THE PR THREAD: this PR went through ten rounds of internal adversarial review. Sixteen findings, none a code defect after round 2 -- every one was in measurement prose carried inside docstrings, which is what the records-protocol pass above removes. The thread is the litigation; this commit is the change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
oshaughnessy-junior
deployed
to
private-review-dispatch-rift-upstream
August 20, 2026 01:51 — with 
GitHub Actions
Active
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
3 participants
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.
Purpose and merge order
This is the second-stage synchronization of
rift_O4dfromoshaughnessy-juniorintooshaughn.#173 was merged first with merge commit
5f1b4b65, as required; merge this PR using a merge commit too. #173 carries the reviewed O4c/master consolidation and two final consolidation commits that are not on the fork'srift_O4dtip. This PR now advances the branch through the work merged on the fork since its paired consolidation PR, oshaughnessy-junior#91.The order has been checked locally against the exact remote tips. A synthetic merge of #173 followed by
oshaughnessy-junior:rift_O4dcompletes without conflicts; Git only performs normal auto-merges inCHANGES.rstandutil_RIFT_pseudo_pipe.py.With #173 on the base, the fork-side delta after the #91 consolidation merge is 168 commits, 138 files, +24,264 / -441.
What this adds after #173
Likelihood interpolation, slow rotation, and JAX correctness
Sampler records, reproducibility, and LISA parity
--seedreproducible on GPU, closes remaining reachable unseeded RNG sites, and folds calibration RNG into one derived-RNG counter registry (#103, #119, #127).Pipeline, Asimov, and simulation-management updates
gp_linmeantracer-placement fitting and an optional lnL floor (#105).simulation_managerdeduplication after archive reopen by normalizing the JSON lookup-key contract; adds append-only input/output transfer hooks; and exposes the OOM hold policy as arguments rather than constants (#107, #111, #138).Review and validation status
#173 -> oshaughnessy-junior:rift_O4d.oshaughnbase before it is marked ready.Reviewer checklist
5f1b4b65).oshaughn:rift_O4dbase: 168 commits, 138 files, +24,264 / -441.