Sync current oshaughnessy-junior rift_O4d after #173 consolidation - #178
Draft
oshaughnessy-junior wants to merge 178 commits into
Draft
Sync current oshaughnessy-junior rift_O4d after #173 consolidation#178oshaughnessy-junior wants to merge 178 commits into
oshaughnessy-junior wants to merge 178 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>
…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
RIFT/likelihood/test_slowrot_*.py ran in NO CI job: `grep -rn slowrot .github/workflows/ci.yml` returned one hit and it was a comment. That left the deliverables of #163 and #165 unprotected -- both are changes whose product IS the guard, and #165 provably moves no number, so an unrun guard leaves nothing. Adds .travis/test-slowrot.sh and a slowrot-check job, modelled on .travis/test-jax.sh, which solved the same problem for test/jax/. Explicit file list, collected-count floor, hard fail on any nonzero pytest exit, and a junit outcome assertion, because a bare `pytest <dir>` here would be worse than nothing: five test_slowrot_*.py files collect ZERO items and exit 5, "no tests ran", which reads as a pass. Four tiers, each measured rather than assumed: TIER 1 41 pytest tests over 9 files. TIER 2 3 module-scope scripts, run as `python <file>` and required to exit 0. They assert at import time and define no test_* function, so pytest gives them no count and no junit row while executing them twice. DESELECT test_W5_jax_packer_loses_nothing. Without jax it does not skip -- it returns early and REPORTS PASSED, so leaving it in would add 1 to the floor and gate nothing. The floor is 41, not 42, because of it. EXCLUDED the two GPU files (measured: 2 skipped, exit 0, on a CPU node) and the two print-only studies that contain zero assert statements. Separate from jax-ile-check on purpose: numpy + lal only, so a numpy failure is not diagnosed behind a jax/numpyro install or an unpinned upstream jax release. test_slowrot_fd_ops.py is gated HERE, not in q-window-stencil-check. PR #166, opened before this job existed, adds it there; a comment in that job says to delete its line rather than run the file twice. Cost, measured end to end through the script (RIFT_develUWM python 3.8, numpy 1.23, OMP_NUM_THREADS=1, no GPU): 70 s on citlogin6 (AMD EPYC), 350 s on ldas-pcdev11 (Intel Xeon E5-2630 v4). Neither host is the runner. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
F4 was a real defect, not prose: the two tiers resolved `import RIFT` by different mechanisms. TIER 1 runs under pytest, which prepends .../Code to sys.path and so tests the CHECKOUT; TIER 2 runs each script directly, so sys.path[0] is RIFT/likelihood/ and `import RIFT` fell through to whatever was INSTALLED. Measured with PYTHONPATH unset: pytest resolved RIFT to the checkout, the scripts resolved it to ~/RIFT_develUWM/src/research-projects-RIT -- a different tree entirely. Benign in CI (the job pip-installs --editable ., so both agree, and the runner log confirms it), but the script is also the documented way to run the gate by hand, and by hand the tier carrying the ONLY module-scope asserts validated a different checkout. Fixed by exporting PYTHONPATH after the cd; a no-op in CI. The other three substantive findings were prose, which is where the sibling PR #166 spent eleven of its thirteen review findings: F1 The ldas-pcdev11 cost row did not sum and its total was not reproducible. It claimed 350 s whole-gate with 212 s + 128 s of tiers -- but 212 + 128 already exceeds 350, and the text said the 350 INCLUDED the collection pass. The 350 was never measured: it was a sum of components timed separately. Three fresh whole-gate runs across two independent sessions give 377 / 411 / 423 s. That row is now a RANGE and is deliberately not decomposed -- pcdev11 is a shared interactive node whose run-to-run spread (~46 s) exceeds the precision a breakdown would imply. The citlogin6 row does reproduce and does sum, and now says so explicitly. F2 "the only automated guard on the arrival-time post-phase" was false. MEASURED: test_slowrot_noloop_bruteforce.py fails on BOTH post-phase mutation shapes too -- the inline-identity shape, and the model-norm-only shape that is the true #159 defect (which trips the bound directly: "Cauchy-Schwarz VIOLATED: max lnL exceeds 0.5<d|d> by 10.5658"). The original claim survived because the tier stops at the first failing script, so the mutation run never reached the second one. Corrected in both the PR body and this script. F7 Records the OBSERVED runner cost, as the adjacent jax-ile-check comment already does: 41 passed in 37.94 s, gate step 65 s, job wall 2m02s, on python 3.10.20 / pytest 9.1.1 / numpy 2.2.6 -- a newer stack than any measured locally. That is now the leading figure; the local hosts are context. F5 "the Intel timings below" pointed at timings this file does not contain. F6 --deselect is a PREFIX match, so a future test_W5_..._v2 would be swallowed silently and the >= floor could not see it. Documented. F8 mktemp was the one command whose rc was unhandled, against this script's own stated rule. Guarded. Also records the #166 merge arithmetic: that PR grows test_slowrot_fd_ops.py from 7 tests to 9, so whichever merges second raises 7/41 to 9/43 in the same commit. Raising it early is the failure that goes red; leaving it late only leaves the floor weaker than it could be. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…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>
Round 2 of the adversarial review found no code defect and nine
documentation findings -- three of them newly-wrong numbers in the commit
that fixed wrong numbers. Patching them would have reset the clock, so
this applies the records-protocol cleanup instead: the numbers that kept
going stale had no business in a comment.
The tell was explicit. A comment a later PR has to edit to keep it true
IS a record, mis-homed. The ci.yml cost table was edited once already
and round 2 asked for three more edits to it:
- "~28x the observed runner wall" compares the wrong quantity.
timeout-minutes is job-level, so it bounds the job wall (122 s and
140 s on the two runs = 13-15x); 28x was 1800/gate-step.
- the recorded runner observation was from the superseded commit, and
the run on HEAD differs by a third (TIER 1 37.94 s -> 51.60 s).
- the citlogin6 row's "67-70 s" excluded three later runs on the same
host (75.1/76.4/80.6) and three more today (79.4/71.8/71.0).
All three are the same defect: a wall-clock table in a comment, which
rots silently because nothing imports it. So ci.yml now keeps the
decision (timeout-minutes is a runaway backstop, an order of magnitude
above the observed job wall) and points at PR #172 for the measurements.
Same pass over .travis/test-slowrot.sh, 319 -> 268 lines, 59% -> 51%
non-code:
- drops "worth 0.207 nats on a real p_max=2 bank" -- a measured number
restated from a docstring I did not measure it in, and that docstring
is being distilled to a third of its length on #166. The code keeps
the decision and cites #163, which is immutable; a docstring line
number is not.
- drops the per-file test counts. They need maintenance on every test
added (#166 already invalidates them), and --collect-only answers it.
EXPECTED_TESTS is the pinned constant; the manifest keeps the list
complete.
- drops the history narration ("an earlier revision zeroed every p>=1")
and the review-harness vocabulary that leaked in ("the sandbox",
"~/RIFT_develUWM" -- one developer's home path, in a repo file).
- keeps and sharpens the anti-instructions, which is what comments are
for: do not simplify to a bare pytest, do not tidy the module-scope
asserts into functions, must PREPEND not append, name any W5
successor differently, never lower the floor silently.
- states the invariant the PYTHONPATH export establishes (this gate
always tests THIS checkout, never an installed build).
- de-twins the #166 merge arithmetic: it lived in two files, now once,
at EXPECTED_TESTS, with a pointer from ci.yml.
Also records what round 2 verified that this commit does not change: the
SCRIPTS tier's three files differ in assert SCOPE, and that decides how
each is gated -- two assert at module scope and are gated twice (here and
by pytest collection, where a failed module-scope assert is a collection
error), the third asserts inside a function reached only via __main__ and
is gated only by execution. Each was mutation-tested separately against
its own unmutated baseline.
CORRECTION to the previous commit message, which said "212 + 128 already
exceeds 350": it does not, 212 + 128 = 340. The pcdev11 row was
inconsistent for the opposite reason -- it left 10 s for a collection
pass that costs ~37-41 s on that host. The conclusion (the row was a sum
of separately-timed components, never a measurement) stands.
This commit changes COMMENTS ONLY. Verified: the executable lines of
test-slowrot.sh are byte-identical to the reviewed version, and ci.yml's
diff contains no non-comment line. Gate re-run green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ds-cleanup slowrot_freqresponse: distil the Nyquist docstrings; route the evidence to the paper repo
Round 3 found no code defect but four text findings, one of which the
previous commit INTRODUCED, and it is the worst kind: a false statement
about the gate's own architecture, sitting directly under the
anti-instruction that protects the only guards on the arrival-time
post-phase.
The claim was that the two module-scope SCRIPTS files are "gated TWICE:
here, and by pytest collection, since a failed module-scope assert is a
collection error". They are not. SCRIPTS appears only in the manifest
loop, the execution loop and the final echo; BOTH pytest invocations take
"${FILES[@]}" only, and there is no conftest.py at RIFT/likelihood/,
Code/, or the repo root. pytest never imports these files.
How it got in is worth recording, because the mechanism is not a typo. A
peer session measured that breaking a module-level assert in
test_slowrot_cauchy_schwarz.py yields "Interrupted: 1 error during
collection", and I wrote that into the comment as a property of THIS
gate. Their measurement is correct -- for THEIR invocation, which aims
pytest at the file. Mine never does. Re-measured both ways:
pytest --collect-only over FILES (what this gate runs) exit 0
pytest --collect-only aimed at the broken script file exit 2
So the corrected comment says all three are gated ONLY by execution, and
the anti-instruction now rests on the consequence that is actually true:
a function this tier never calls leaves `python <file>` exiting 0 having
asserted nothing. The consequence was always right; the mechanism was
borrowed from someone else's configuration.
Also fixes a twin the same cleanup left standing. ci.yml said "three of
those five assert at module scope"; only two do, and the previous commit
corrected exactly this in test-slowrot.sh while leaving the ci.yml copy
untouched -- one PR asserting two incompatible facts about the same three
files. Swept .travis/ and .github/ for further copies; none remain.
NOT comment-only, unlike the previous commit: three runtime message
strings also over-claimed module scope ("Add it to SCRIPTS if it asserts
at module scope", "== TIER 2: module-scope assert scripts ==", "This file
asserts at MODULE SCOPE") and are widened to "outside a test_* function".
Message text only; no control flow, no file lists, no thresholds touched.
Gate re-run green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…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
…e floor to 43 #166 merged first, so the coordination step agreed with its author falls to this branch: - DELETED test_slowrot_fd_ops.py from q-window-stencil-check. It is gated by slowrot-check, whose manifest requires every test_slowrot_*.py in that directory to be listed or explicitly excluded; a copy in another job's list is invisible to that manifest and would simply run the file twice. - RAISED EXPECTED_TESTS 41 -> 43. #166 added two known-answer tests to test_slowrot_fd_ops.py (test_nyquist_guard_clauses_on_synthetic_axes and test_rotation_post_phase_is_not_the_identity, the latter being issue #173's helper). The MERGE NOTE that recorded this arithmetic is retired now that it has been applied. Counts re-derived on the merged tree rather than carried from the handoff message: test_slowrot_fd_ops.py collects 9, TIER 1 collects 43 after the one deselect, and the directory still holds 16 test_slowrot_*.py files, so the manifest is unaffected. Gate re-run green: 43 passed, junit tests=43 skipped=0 failures=0 errors=0, PASS (43 tests + 3 assert scripts). The ci.yml conflict was exactly the coordination point -- this branch's "these files do not belong here" comment against the base's newly-merged fd_ops entry. Resolved to this branch's text, minus the now-spent instruction to whoever merged second. Checked two hazards flagged by #166's author and found neither present here: nothing in this branch quotes the 4e-03 figure (which is the two-defect number, not the Nyquist bin alone), and nothing cites the now-distilled docstrings by line number -- the 0.207 attribution was moved to PR #163 before the merge, which that distillation has now made load-bearing rather than merely tidier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
oshaughn
deployed
to
private-review-dispatch-rift-upstream
August 20, 2026 07:51 — with
GitHub Actions
Active
GitHub Actions
Active
…ence-normalization CIP: normalize final evidence against the configured prior
oshaughn
deployed
to
private-review-dispatch-rift-upstream
August 20, 2026 23:44 — 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.