Sync junior rift_O4d into upstream rift_O4d - #188
Merged
Merged
Conversation
create_event_parameter_pipeline_BasicMultiApproxIteration evaluates several
waveform models on ONE shared intrinsic grid and marginalizes over models at
each grid point,
L_marg(lambda) = sum_m p(m) L_m(lambda)
so the adaptive grid refines on the model-marginalized posterior. That is the
difference from running N single-model pipelines and mixing posteriors
afterwards: there, each run's grid refines on its own model's posterior, and no
post-hoc mixing recovers support neither run sampled.
Prior work by hand: Yelikar; Jan. The claim is the production workflow, not
the idea of waveform marginalization.
MOST OF THIS WAS ALREADY HERE. util_CleanILE.py keys on the intrinsic
parameters, so the same lambda evaluated under several models collapses to one
entry, and it combines them LINEARLY in L -- which is model marginalization, not
an lnL average (that would be the geometric mean, a logarithmic opinion pool,
and is not Bayesian). The loop ILE also read an UNTAGGED overlap-grid, i.e. one
grid shared by every model. The workflow was a half-finished shared-grid
marginalization; the per-model CIP fork inside the iteration loop was the part
that did not belong.
THE LOOP MERGES. Every model's ILE runs on the shared grid and consolidates per
model; one unify pools the composites and marginalizes; one CIP fits that net
and writes the single grid every model reads next iteration.
THE TERMINAL STAGE FORKS. Per-model unify -> per-model CIP (posterior AND ln Z
via --fname-output-integral) -> per-model extrinsic chain -> a new tool,
util_CombineApproximantPosteriors.py, mixing them with weights p(m) Z_m. The
per-model posteriors stay on disk deliberately: that is what a systematics study
looks at.
WEIGHTS ARE A PRIOR, NOT SAMPLING EFFORT. util_CleanILE.py gains
--model-group-regex, switching on a two-level combine: ntot-weighted within a
model (replicas of one quantity), p(m)-weighted across models (the quantity
itself differs). Flat pooling silently used replica COUNTS as model weights.
On a two-model point with lnL 100 (two replicas) and 104 (one), flat gives
102.937 where uniform-prior marginalization gives 103.325 -- 0.39 nats, growing
with model disagreement and replica imbalance. Opt-in; ONE model reduces to the
previous behaviour byte-identically, and the default path is unchanged (verified
against the pre-change file).
PARTIAL COVERAGE IS REPORTED. A lambda evaluated under only some models is
marginalized over that subset, so the estimator changes point to point -- a
model- and lambda-dependent change in the effective prior, caused by the
sigmaOverL>0.9 cut firing for one model and not another, or a model's ILE
failing. Always warned; --require-all-approx drops such points instead. That
is not free either (it biases toward where the worst-resolved model converged),
so it is a choice, not a default.
BUGS FIXED ALONG THE WAY, each confirmed on an emitted two-approximant DAG.
None could be seen with one approximant, and this builder has no caller
(--pipeline-builder does not offer it, and asimov drives pseudo_pipe), so
nothing exercised them:
* the terminal extrinsic stage read overlap-grid-<n_iterations-1> while the
final CIP wrote overlap-grid-<n_iterations>: `if not ('it' in globals())`
preserved an `it` the loop had already left one short. At one iteration it
read the raw seed grid.
* join_grids.sh interpolated $(macroapprox) INSIDE A BASH SCRIPT -- command
substitution, not a condor macro, since the .sub passes only $1 $2. It
matched nothing. Gone now the loop grid is not model-tagged.
* CIP.sub's initialdir named iteration_N_cip while the mkdir loop created
approx_<A>_iteration_N_cip; the consolidate/unify log directories were the
mirror image. Either holds the job on the execute node.
* parent_fit_node was one variable spanning `for it: for approx:`, so model
B's iteration-0 ILE waited on model A's iteration-1 convert. The models
were serialized and cross-coupled; they now run in parallel.
* the terminal convert wrote one untagged posterior_samples-N.dat from every
model, while the convergence test read a per-model name nothing wrote.
SHARED CODE. write_unify_sub_simple gains script_name and glob_pattern (a
workflow needing two unify jobs otherwise has the second overwrite the first's
script); write_convert_sub no longer appends the literal string "None" when a
tool has no trailing positional. Both default to previous behaviour: building
BasicIteration with and without this change gives 0 of 27 emitted files
differing.
TESTS. test/test_multiapprox_marginalization.py, 16 cases, ~11 s, self-
contained (it writes its own args and seed grid, since the builder has no
caller). Two groups: the combination arithmetic against hand-computed values --
including a guard that we do NOT compute the geometric mean -- and the emitted
DAG's shape. The DAG cases build TWO approximants deliberately; with one, every
cross-model path is vacuously satisfied, which is how the defects above
survived. test_every_job_directory_exists resolves each node's own macros, so a
missing directory OR an unresolved $(macro) fails -- the latter is how ILE_extr
came to interpolate an empty approximant into both --approx and its initialdir.
Added to the ci.yml job that carries the CIP prior suite: this workflow runs
explicitly-named files, so a new test file otherwise runs nowhere.
NOT VALIDATED ON DATA. These are build-shape and unit-arithmetic checks, not
physics. The prototype is a rerun of one of the Jan/Yelikar cases -- cheap, and
with large between-model differences -- framed as a reproduction claim first and
an efficiency claim second. Nothing here belongs in a paper until that runs.
Design record: RIFT/misc/DESIGN_multiapprox_marginalization.md.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The multi-approximant builder had no caller: --pipeline-builder offered only
BasicIteration, AlternateIteration and Hyperpipe, and asimov drives pseudo_pipe.
That is why its defects survived, and it also meant a cross-model run had to
hand-write args_ile.txt / args_cip_list.txt / args_test.txt -- exactly the
settings helper_LDG_Events.py already writes for production.
Now:
util_RIFT_pseudo_pipe.py ... --approx SEOBNRv5HM --approx-extra IMRPhenomD
--approx-extra is repeatable and IMPLIES --pipeline-builder
BasicMultiApproxIteration (asking for a second waveform model is asking for the
cross-model builder; requiring the user to say it twice only invites them to
disagree with themselves). --approx-prior and --require-all-approx pass through.
The models reach the builder as repeated --approx, so every one is evaluated on
the SAME shared grid.
The point is not convenience. It makes a cross-model analysis production-matched
BY CONSTRUCTION -- PSD, cache, channel names and CIP settings come from the same
helper that writes them for a single-model production run, instead of being
chosen by whoever set up the comparison. On this project four confident
conclusions turned out to be artifacts of an unmatched stand-in.
API COMPATIBILITY, THE HONEST WAY. pseudo_pipe emits one command line shaped for
BasicIteration's option surface, which declares 63 options this builder does not
implement -- and one of them, --n-iterations-subdag-max, is emitted
unconditionally, so the builder died on it immediately.
This builder now ACCEPTS all 63 (hidden from --help) and REFUSES any that is
actually used, naming it. It does not ignore them. A flag that parses and does
nothing changes a run without saying so and is invisible in a config diff, which
is a worse failure than crashing. Verified both ways: a pseudo_pipe-shaped line
builds, and the same line with --n-iterations-subdag-max 5 exits 1 with the
option named.
test_multiapprox_pseudo_pipe.py re-derives the option surfaces from the two
builders' sources and fails if BasicIteration ever declares something this
builder neither implements nor explicitly refuses. Without that, the next option
added upstream breaks cross-model runs at DAG-build time in someone's campaign,
and the tempting fix -- ignoring it -- is the failure above. Added to ci.yml,
which runs explicitly-named files.
109 tests pass across the multi-approximant suites and the CIP prior suite.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… gate registration pending)
Two exact angle-marginalization schemes for the distphipsimarg path, both
built on the analytic fact that lnL_t at fixed (t, distance) is a bivariate
trig polynomial (phi order <= 2*m_max, u=2psi order <= 2): 'exact'
(Nyquist-sized coefficient bootstrap + dense reconstruction) and 'laplace'
(analytic psi-Laplace, error O(1/A)). Wrapper/driver get
--angle-marg-scheme {grid,exact,laplace,auto}; DEFAULT stays 'grid'.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n setup Driving a real cross-model run from pseudo_pipe -- --event-time + --fake-data-cache + --use-online-psd-file, i.e. the natural synthetic-injection path -- turned out to be broken in ways only running it exposes. All four are pre-existing except the last, which is the new builder meeting the driver. 1. NameError: name 'config' is not defined. --use-online-psd-file read the ini's [analysis] ifos UNCONDITIONALLY, but `config` only exists on the --use-ini path (the commented-out lines above it show where it used to be built). So the flag was unusable without an ini. Now: the ini when there is one, else --manual-ifo-list, else the PSD file's own instruments -- which cannot disagree with the file being used. 2. KeyError: 'IFOs'. --fake-data-cache indexes event_dict["IFOs"], which is populated only by the ini path and the gracedb lookup -- never by --event-time. Resolved from --manual-ifo-list or the instruments already read off the PSD file, and stored so the later consumers of event_dict["IFOs"] see it too. 3. The pipeline builder's exit status was discarded. `os.system(cmd)` with no check, so a builder that refused its arguments left pseudo_pipe reporting SUCCESS with every args_*.txt present and no DAG in the run directory -- which looks exactly like a finished setup. Observed: the builder exited 1 and pseudo_pipe exited 0. Now checked, with the reason printed. 4. os.system(cmd) where os.system(cmd_enough) was meant, inside --internal-ile-check-good-enough: it built the `find ... -exec touch` command and then re-ran the PIPELINE BUILDER instead, rebuilding the whole DAG. Also, two options are no longer emitted to the multi-approximant builder: --n-iterations-subdag-max (a subdag concept; it was emitted unconditionally, so the builder refused every cross-model run) and the two --last-iteration-extrinsic-samples-per-ile controls, which it does not implement. The latter prints a notice naming what was dropped rather than changing the extrinsic sample count in silence. VERIFIED END TO END. pseudo_pipe now writes the args_*.txt via helper_LDG_Events.py and builds a cross-model DAG: 566 jobs without the extrinsic stage, 8773 with it. Structure confirmed on the emitted DAG -- both models on the loop ILE, SEVEN CIP nodes none of which carries macroapprox (one fit per iteration over the marginalized net), unify waiting on every model, and the terminal fork present as 2 unify_model + 2 CIP_terminal + 2 cat + 1 combine_models. Sizing note for the record: 8000 of those 8773 jobs (91%) are the convert_extr/resample pair, at 2 x n_output_samples_last per model. That is the deprecated per-point extrinsic path quantified for this workflow, and it is why teaching this builder fairdraw-extrinsic is the prerequisite for running it at campaign scale. 21 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
simulation_manager: container universe and output-transfer timing
…emory defaults - register test_angle_marg_exact.py in .travis/test-jax.sh (FILES + ledger), EXPECTED_TESTS 139 -> 163 by collection; ci.yml cost notes updated. - record the measured injection-ladder calibration (SEOBNRv4 35+30 HLV, SNR 10-80) in the crossover-constant note; laplace-exact falls from -1.1e-3 (A=50) to -2.8e-6 (A=3200) while the legacy grid errors grow to -27 (32x8) / -51 (8x8) nats. - laplace kernel: guarded log-add-exp and branch-safe inputs (jnp.where VJP sends a zero cotangent through the unselected branch and 0*inf = nan), Newton under stop_gradient + one differentiable polish step, curvature floors relative to amplitude; kernel FD-gradient and O(1/b) error-law tests added; scheme-level laplace gradient checked AD-vs-AD against the exact scheme (measured ~5e-4 relative). - exact scheme memory defaults dense_chunk=8, grid_block=32 (inner slab ~0.8 GB f64 at batched S=64, npts=614). - full gate run: PASS, 163 tests, 0 skipped, 981 s local. - mutation audit: 8 mutations (DFT sign flip, dropped harmonic row, halved sample grid, dropped normalization, single stationary start, flipped selector, inert wrapper flag, dropped driver kwarg) each fail >=1 test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All three confirmed against the source and fixed; the third also had a gap in my own gate, which is closed. P1: THE TERMINAL FIT USED THE WRONG ARGUMENTS. cip_args is cip_args_lines[0], but the iteration loop advances through the list via cip_job_list, so with a multi-line --cip-args-list the run finishes on a LATER configuration. The terminal per-model CIP was built from cip_args, so the per-model posterior and evidence could use different coordinates, fit method or resolution than the iteration that produced the grid they fit -- and those evidences are what the final mixture is weighted by. Now expands cip_args_n into a per-iteration list and takes the entry for iteration n_iterations-1. P1: MODEL DISAGREEMENT WAS BEING ENCODED AS INTEGRATION ERROR. The cross-model combine folded the between-model scatter into sigmaOverL, on the reasoning that it is the waveform-systematic contribution and should widen the downstream fit. It does the opposite. sigmaOverL is an INTEGRATION error and CIP drops every row above --sigma-cut (default 0.6, util_ConstructIntrinsicPosterior_GenericCoordinates.py:320,2211), so scatter large enough to matter DELETES precisely the intrinsic points where the models disagree -- the points this workflow exists to fit. Measured on the two-model test case: scatter 0.964, i.e. above the cut, so that point would have been thrown away. Across models sigmaOverL is now the propagated per-model integration uncertainty only; the model variation is already carried by Lbar. The scatter is still computed and REPORTED as a diagnostic, never folded in. (Within a model the scatter term is correct and is unchanged: those are replicas of one quantity.) P1: THE PLOT JOB NAMED A MODEL IT NEVER BINDS. plot.sub's log paths contained $(macroapprox) while plot_node supplies only the iteration macros, so with plotting enabled the macro never resolves and the job has no valid log directory. The plot node is model-independent -- it summarises the run once -- so its log paths and its mkdir are now model-independent too. AND THE GATE MISSED IT. _assert_multiapprox_job_directories_exist checks for exactly this, but plotting is off by default and the test never passed --plot-args, so the plot stage was never built and no assertion could see it. A stage that is not built is a stage nothing checks. The test now builds with plotting on; mutation-tested, it fails with "plot.sub: unresolved macro in ...approx_$(macroapprox)_iteration_2_plot/...". 21 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_dag_chains_levels_per_sim asserted `len(list(dags_dir.iterdir())) == 1`. Where condor_submit_dag is actually installed it runs and drops four companion files next to the DAG -- .condor.sub, .dagman.log, .lib.err, .lib.out -- so the directory holds 5 entries and the assertion fails. The test's own comment says "condor_submit_dag missing -> noop dispatch", which is the tell: it was written on a machine without HTCondor and is green only there. There is exactly one DAG either way; the product is fine. Glob for *.dag instead, and put the directory listing in the assertion message so the next person sees immediately what is there rather than just a count. This was the one red test on rift_O4d, and it has been cited as "pre-existing, not mine" in three separate PRs while nobody looked at it. Whole suite green now: 227 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…a-derived sizing)
Defect 1: the psi-Laplace kernel seeded Newton only at the FIRST harmonic's
extrema (u0 = beta, beta+pi), which fails outright when that harmonic
cancels -- c1=0, c2=-d, d>0.5 put both seeds on MINIMA and returned -inf
for a finite integral. Rewritten angle-free with ALL stationary points
enumerated: f' is a degree-2 trig polynomial (resultant quartic in e^{iu}),
so it has at most 4 transversal zeros; a 24-cell sign scan brackets each
(interval-based -> no duplicate roots, including zeros exactly on a grid
node), bisection under stop_gradient converges them, one differentiable
Newton polish step carries the implicit derivative, and near-degenerate
maxima (|H| < h_floor) are kept with floored curvature instead of dropped
(-inf can no longer be returned for a finite integral). Measured: the
counterexample family now errs O(1/d) (0.16 at d=0.7 down to 2.5e-4 at
d=500); a 200-draw randomized (b,d,beta,delta) sweep including b<<d gives
worst |err|*(b+d) = 1.75; gradients FD-exact including at b=0.
Defect 2: dense-grid sizing keyed on guess_snr, so a missing or
underestimated SNR silently pinned amp_sizing at the 450 floor and
under-resolved the quadrature (measured -1.04 nats on a kappa_boost=200
synthetic with true amplitude ~1.7e4) -- the n_psi=8 failure mode again.
Sizing and selection now key on estimate_angle_amplitude: a DATA-DERIVED
bound from the coefficient tables themselves (eager 64-point sky scan at
wrapper build, max over sky/time/actual distance nodes of
(x*M_A - x^2/2*B0)+, margin 2), unfloored for selection and floored at the
crossover for sizing. The fused functions now REQUIRE amp_sizing (raise on
None -- never a silent default); guess_snr no longer influences either.
Both regressions verified to bite pre-fix (kernel -inf repro; floor-sized
wrapper off by 1.04 nats) and are pinned in 5 new tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…9 (review item 4) The test file grew by 5 (the two review-defect regressions plus kernel sweep/no-default/estimator pins) while EXPECTED_TESTS still said 163; the floor is >= so the gate passed while leaving the new tests unprotected. Recomputed by pytest --collect-only (168) in the same commit as the ledger text, per the gate's own rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…uard, empirical amplitude estimator Item 2 (confirmed): the hard series/Laplace switch at b+2d=0.5 left a wide bad window (0.27-0.53 nats value error, SIGN-INVERTED |c2| gradient across b+2d in [0.5,~5]). The Bessel series is extended to cross-term order k=8 with each I_n a fixed 26-term power series (elementary ops, inputs clamped so the polynomial cannot overflow on 0-weighted bins), phases division-free via powers of w = c2*conj(c1)^2, and the two branches are blended C1-smoothly over b+2d in [10,16]. Measured: machine precision through t=10 (the review's probes at 0.5001 and 2.0 exact in value and gradient), O(1/A)-bounded and sign-correct through the band (worst 0.17 val / 0.47 grad on adversarial draws at t~15 -- bins that are exp(-(A-16))-subdominant in any marginal the laplace branch serves), max step-to-step blend jump 2.4e-4. Pinned by a window test that does NOT filter the band out, and the randomized sweep's low-amplitude filter is removed. Item 3 (confirmed): the driver AST guard accepted any angle_marg= keyword, so the inert-flag mutant angle_marg="grid" at the call site passed the suite. The guard now requires the keyword's VALUE to be the angle_marg variable and that variable to be read from opts; both the reviewer's mutant and the re-hardcoded-variable mutant now fail it (verified). Item 5 (suspected, addressed): estimate_angle_amplitude's analytic M_A-with-mean-B expression is heuristic in exactly the reviewed direction. The PRIMARY estimate is now the EMPIRICAL max of the exponent over a dense 96x24 angular reconstruction (near-exact for the band-limited trig polynomials; closed-form concave-in-x distance max per point); the analytic expression is kept as a runtime cross-check that prints loudly if it ever reads below the empirical max. Docs corrected: auto engages from true A ~ 225 (SNR ~ 21) because the margined bound is compared to the crossover. Item 1 was fixed in 6600060 (the selector floor bug -- caught by this suite when run); item 4 in 5d3f9c2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d asserted A mutation audit found the estimator's hardcoded 96x24 angular grid was unpinned: a gutted grid read within 2% of a 512x128 reference on the broad- peaked test target (and a finer-grid self-comparison was common-mode blind to the same mutant). The grid is now DERIVED from m_max and asserted fail-closed, mirroring angle_sample_grid_sizes; the estimator test gains an INDEPENDENTLY-computed dense reference (not routed through the estimator). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…pdated Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Running the first real cross-model DAG found that it could not do the comparison
it exists for. With --use-gwsignal applied globally -- which is what
util_RIFT_pseudo_pipe.py emits -- every IMRPhenomD ILE job died with
ValueError: Invalid Argument (gwsignal gen_modes)
XLALSimInspiralGenerateTDModes: generator does not provide a method to
generate time-domain modes
and contributed ZERO rows. util_CleanILE then saw ONE model, and the run
silently degraded to single-model with no marginalization at all. Measured
across families:
SEOBNRv5HM gwsignal OK (4 modes)
IMRPhenomD gwsignal FAILS
IMRPhenomXAS gwsignal FAILS
IMRPhenomXPHM gwsignal FAILS
The phenom family has no time-domain mode generator in gwsignal; SEOBNRv5* is
only available there. So a single global flag cannot serve an EOB-vs-phenom
comparison, and that is exactly the comparison this builder is for.
New --approx-gwsignal (repeatable) names the models that need gwsignal. The
builder strips any global --use-gwsignal out of the ILE arguments and binds the
route as $(macrogwsignal) per node, so one DAG mixes families:
SEOBNRv5HM -> " --use-gwsignal "
IMRPhenomD -> " "
Gated by test_generator_route_is_per_model, which fails if ILE.sub carries a
global --use-gwsignal instead of the per-node macro.
HOW THIS HID, and worth stating because the shape recurs: a model contributing
zero rows is INVISIBLE to the partial-coverage check, which compares only the
models it has SEEN. "model-aware combination over 1 models" reads like status,
not like an error. Making util_CleanILE take the expected model list and fail
when one is entirely absent is the follow-up.
22 tests pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…om the sample rate
The time integral of the marginalized likelihood is taken with Simpson's rule at
the FIXED spacing deltaT = 1/srate. That spacing is a property of the DATA; the
width of the integrand is a property of the SIGNAL,
sigma_t = 1 / (2 pi rho sigma_f)
and shrinks as 1/rho. Production therefore under-resolves its own integrand,
worse at higher SNR, and Simpson's (4 T_h - T_2h)/3 form makes an under-resolved
peak worse than trapezoid rather than better, because it carries the coarser
T_2h and inherits its 2h alias.
kappa(t) is band-limited below Nyquist by construction (the rholm timeseries are
inverse FFTs of a product truncated at fmax <= fNyq) and rho_sq is
time-independent on this path, so the samples the code ALREADY computes determine
the continuous integrand exactly. One zero-padded FFT per row recovers it: no
extra likelihood evaluations, no extra precompute, no extra accumulator passes.
Adds an opt-in 'bandlimited' quadrature. THE DEFAULT IS UNCHANGED: 'simpson'.
The refinement factor is derived from the measured peak width and re-asserted on
the refined grid -- there is deliberately no resolution option, because this
defect class exists because a resolution was once a settable number. Restricted
to n_cal == 1 on the non-rotating likelihood; every other path refuses rather
than silently applying an argument that does not hold there.
Scope is DiscreteFactoredLogLikelihoodViaArrayVectorNoLoop only. NoLoopOrig and
DiscreteFactoredLogLikelihoodViaArrayVector keep Simpson unchanged, knowingly.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… branch selection, and honest language about the amplitude bound Three defects from a third independent review: 1. The final dense quadrature ignored m_max. The previous fix made the COEFFICIENT RECONSTRUCTION grid scale with mode content, which looks like it covers higher modes but does not: integrating exp(lnL) needs phi resolution proportional to the highest harmonic 2*m_max, ON TOP OF the ~sqrt(A) amplitude density. _dense_grid_sizes() only ever saw the amplitude. Measured with the 2x margin: a pure order-8 term at true A=450 was phase-dependently wrong by ~+-0.037 nats, and order 16 ranged -1.20 to +0.57. _dense_grid_sizes now takes m_max and scales n_phi by it. This survived two earlier reviews because higher-mode coverage stopped at the reconstruction step and never tested the FINAL MARGINAL. 2. Branch selection was global where the kernel is local. The tolerance for branch-window error assumed bins at t~10-16 are exponentially subdominant whenever the GLOBAL amplitude exceeds the crossover -- but selection happens once, from a maximum over sampled sky positions, while the kernel runs at EVERY proposed sky position. At a low-response proposal, moderate-t bins can be locally dominant: b=11.8866, d=3.5163, beta=-1.8900, delta=-0.6497 gives t=18.9 and +0.251 nats against dense quadrature. Selection is now local. 3. The "upper bound" was 64 random sky/inclination draws plus a fixed 2x margin, which is an ESTIMATOR, not a bound -- and the test that checked it reused the SAME sampled positions, so it was blind to a missed sky maximum by construction. The language and the fail-safe now say what it actually is. Committed by the coordinating session: this work sat uncommitted in a shared worktree for several hours, where it had no reflog and was one stray command from being lost. Authorship is the implementing agent's; the commit is mine because it should not have stayed uncommitted.
The edge-guard classification left one gap: a row with no resolvable curvature (an extrinsic sample in an antenna null, where kappa is numerically zero) was excluded from the wrap guard -- correctly, so the diagnostic stops reporting "mis-centred window" for rows that simply have no signal -- but then fell through to the dense path at factor 1, which integrates on the COARSE grid with TRAPEZOID rather than Simpson. Numerically a non-event on a featureless integrand, but it silently changes the rule for rows this option was never meant to touch, and it forfeits the one property a reviewer can actually check. Make the rule exact instead: a row is refined only if it has a trustworthy peak AND the derivation asks for a finer grid. Everything else -- wrap-exposed, unmeasurable, or already resolved -- gets the historical Simpson value. So a row's returned value differs from the historical one if and only if its integrand was under-resolved, and `last_report()['n_refined_rows']` is exactly the set of rows that moved. Raised by the parallel rift_O4c effort, which kept the guard blanket and let n_flat_rows overlap it instead; this resolves both the diagnostic and the guarantee rather than trading one for the other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 169-test gate hit the job's 60-minute timeout-minutes cap and was CANCELLED at 65 minutes (run 33121111049), which the PR then displayed as a FAILING check -- indistinguishable at a glance from a broken test. The 139-test baseline ran in 13m53s, so the growth was not gradual-looking from the badge. RO'S: some tests have to be development checks rather than per-commit gates, the way full RIFT analysis runs already are. Deselected (NOT deleted, and not weakened): test_laplace_high_amplitude_accuracy_and_trend scale=100 test_higher_mode_dense_sizing_self_convergence 4x oversized grid The split is drawn on a principle rather than on convenience: EXACTNESS DOES NOT DEPEND ON AMPLITUDE. The low-scale brute-force comparisons that remain gated (scale 2/4/6) are what prove the schemes correct, including test_higher_mode_marginal_vs_bruteforce, which is the coverage the third review asked for on the final marginal. What moves out establishes the ERROR LAW at production amplitude -- a property of the mathematics that does not change commit to commit. Cost is dominated by the dense reconstruction, ~sqrt(A) per axis with A ~ scale^2; the PR's own SNR-320 row records the oversized construction as "13M dense points, eager-CPU intractable". Each entry carries its reason and a copy-pasteable command to run it by hand. EXPECTED_TESTS 169 -> 173 recomputed BY COLLECTION with the deselects applied, never by arithmetic (the file also gained tests in the third-review commit, so the count went UP even though two tests left the gate -- exactly the reason this script forbids doing the sum in your head).
…floor 171
Three fixes from the fourth external review, plus the count it caught.
1. EXPECTED_TESTS was 173 where CI collects 170. I computed 173 by collection
in the igwn python; CI's environment collects fewer, so the floor tripped
before any test ran and the gate went red in 1m3s. The number now comes
from CI's own measurement (170) plus the one test added below. Lesson for
the ledger: "recount by collection" must mean collection IN THE GATE'S
ENVIRONMENT -- a local count is not authoritative here.
2. The undersizing fail-safe warned and then published. A jax.debug.print
inside jit stops nothing: a production run would finish and emit biased
likelihoods, samples and evidence while the "fail-safe" scrolled past in a
log. _runtime_amp_failsafe now RETURNS a poison term that both schemes add
to their result, so the output is NaN when the estimator is undersized. A
NaN cannot be silently consumed; an under-resolved finite number is
indistinguishable from a good one. Still under stop_gradient.
3. Moving test_higher_mode_dense_sizing_self_convergence out of the gate
removed the ONLY test that distinguishes the corrected sizing rule -- my
justification for the split ("exactness does not depend on amplitude") is
false for the dense quadrature, and the old m_max-blind rule passes every
low-scale brute-force test here. Replaced with
test_dense_phi_sizing_must_scale_with_m_max: pure numpy, milliseconds,
closed-form reference (I0), phase-swept. MEASURED at amp=450, b=150,
order=16: blind rule (n=352) errs 4.98e-01 nats, m_max-aware (n=1360) errs
1.17e-10. It carries a self-check that FAILS if it ever stops biting.
Two earlier attempts at this test did NOT bite and were discarded rather
than shipped: a pure order-8 harmonic at b=6 is resolved exactly by the
blind rule (err 8.9e-16). The error only appears once the harmonic
amplitude scales with A, which is the real situation.
…il-open guards Findings from a four-lane adversarial review plus a run on real GPU hardware. WRONG ANSWER, three of five production sample rates. bandlimited_upsample split the spectrum at h = n//2 unconditionally. For ODD n that files the highest POSITIVE frequency under a negative frequency. marginalization_time_grid gives npts = 153 / 307 / 614 / 1228 / 2457 at srate 1024 / 2048 / 4096 / 8192 / 16384 -- odd at three of them, including 16384. Measured against an analytic band-limited truth at factor 4: 1.4e-12 at n=614, but 4.1e-1 at n=613, 5.4e-1 at n=307 and 6.0e-2 at n=2457. Invisible to the obvious test: the reconstruction stays exact AT the samples, and the old fixture used only even npts with an empty top bin. Test is now parametrised over the real production sizes and fills every bin below Nyquist. CRASH, 100% of GPU runs. The likelihood called the helper without passing its own `simps`, so the helper defaulted to scipy's, which raises on a cupy array. Every --vectorized --gpu run of the option died. Found by executing on ldas-pcdev13, not by reading the cupy API; all 46 CPU tests were green. The scipy default now refuses for a non-numpy backend instead of leaving the trap armed. (Related, and PRE-EXISTING: scipy's simpson and the vendored optimized_gpu_tools.simps disagree for EVEN npts, so CPU and GPU RIFT already return lnL differing by up to 0.405 nats. Reported, not fixed here.) BROKE THE STANDARD EXTRINSIC STAGE. The guard raised on return_lnLt whenever the module default was bandlimited. --add-extrinsic-time-resampling maps to --resample-time-marginalization, whose resample_samples() calls with return_lnLt=True and no explicit quadrature -- so the run did the whole integration and then died at export. return_lnLt takes no time integral, so the quadrature is inapplicable there rather than ignored; it now raises only on an explicit request. ABORTED THE RUN ON A NaN. The rho_sq time-independence tripwire used a bare `==`, so `nan != nan` fired it and killed the process, blaming a rotating-response path not in use. NaN rows are normal -- the defensive proposal component draws them deliberately. Compares finite entries only. FAIL-OPEN CEILING. A float factor above 2**63 wrapped to a negative int64, which maximum(factors,1) turned into 1, silently classifying an unresolvable row as "nothing to refine". Clamped before the cast so it raises. CLAIM CORRECTED. "A row's value changes iff it was under-resolved" was false: the log-sum-exp offset moves from a global to a per-row maximum for EVERY row, so rows that underflowed to -inf now come back finite. The guarantee is about the RULE, not the value. The test that "proved" it compared against a per-row-offset helper -- common-mode with the code under test, and every fixture ran one row, where the two offsets coincide by construction. Rewritten against the shipped global-offset expression on a multi-row batch with >745 nats of dynamic range. Also: pass the already-computed coarse lnL instead of recomputing it, and fix two broken assertions in the new simps-handover test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RO'S: hard failing "would kill an entire run", and excision is the worse
outcome. Both are right, and the NaN I added in the previous commit was the
wrong fix -- it CAUSED the harm it was meant to prevent.
Why NaN was wrong. Every consumer of this likelihood FILTERS non-finite
values: flowMC/MALA reject such proposals as invalid, the SMC path drops
non-finite lnL, and write_samples discards non-finite rows. So a NaN over a
hot sky region the estimator missed halts nothing -- it EXCISES exactly that
region and publishes a clean-looking posterior and evidence over what remains.
Invisible mutilation beats visible failure only from the code's point of view.
Why aborting is also wrong. This is a configuration ESTIMATE. Destroying a
multi-hour run over a recoverable condition trades a labelled result for no
result.
So: the value is untouched, the run completes, and the condition is recorded on
the HOST (jax.debug.callback, outside the traced graph, never altering a value).
The driver resets it per EVENT -- batch runs analyze several events in one
process, so event 0 must not label event 1 -- and appends
"SUSPECT-ANGLE-GRID amp_failsafe=TRIPPED worst_amp=... amp_sizing=... scheme=..."
to the provenance line write_samples embeds in the exported file, plus a loud
stderr message naming the recourse. An operator gets a labelled artifact they
can judge, rather than a vanished region or a dead run.
Also removed two `except Exception:` wrappers I had put around the guard calls.
A swallowed NameError there would have degraded a tripped run to "clean" --
the same silent-degradation failure this guard exists to remove. The import is
now explicit and the calls are unguarded.
Tests (the previous one asserted only that a warning printed, so replacing the
poison with 0 would have passed it unchanged):
* finiteness of the undersized result is now asserted, so reintroducing the
poison fails;
* the host-side record is asserted, since without it the driver cannot label
anything and the condition dies with the log line;
* a new driver test pins the import, the per-event reset, the label reaching
the WRITTEN provenance, and the absence of the swallowing except.
EXPECTED_TESTS 171 -> 172 for the added driver test.
The edge guard bounds WHERE, not HOW MUCH. lnL is linear in kappa, so the wrap error in nats scales with amplitude: for a row just outside the guard, measured -8.0e-4 / -8.1e-3 / -8.1e-2 / -0.846 nats at peak lnL 5.3e2 / 5.3e3 / 5.3e4 / 5.3e5 (rho ~ 33 / 103 / 326 / 1031). The companion O4c effort measured the same linear scaling on a different fixture and a different implementation, reaching the same magnitude at the same amplitude -- so this is corroborated across lines, not a single-fixture artefact. Adequate through O4, weaker for 3G. And the guard was justified with a claim that is simply false: that such rows are truncated under either rule. Often they are not -- at 20-60 samples from the edge the peak sits entirely inside the window, and those rows are handed a Simpson value measured 2.87 nats wrong where the reconstruction would have been 0.007-0.02. The guard is deliberately conservative; the crossover where the reconstruction actually loses is nearer 5-10 samples. Also documents what "exactly" is exact ABOUT. The reconstruction is exact for the integrand the code actually forms, which is the true kappa only under time_interp='nearest' (the default). With 'cubic'/'sinc' the gathered values are a fixed FIR filter applied to Q -- still band-limited, so the sampling argument survives, but the refinement then converges to the integral of a stencil-BIASED integrand. Measured at peak lnL ~5300: 'nearest' +0.0002 nats against an analytic truth where Simpson is -521; 'sinc' -2.29 where Simpson is +1.28, with Simpson winning about half a scan over seeds and grid phases. The quoted advantages are for the default stencil, which no version of this said before. CI: a collected-count guard, matching test-slowrot.sh and test-jax.sh. set -e already catches a total collection failure (pytest exits 5), but a silent shrink would read as green. EXPECTED=60 obtained by running collection; verified it passes at 60 and trips at 61. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…local follow-up The PR quoted a dozen measured tables that were not reproducible from the tree -- no harness, no fixture, no pointer. This records the numbers beside the module, names the harnesses that produced them, separates what was measured on the JAX mirror from what was measured on this path, and writes up the peak-local design RO'S redirected to, with its prototype numbers and the merging detail that makes it one algorithm rather than a regime switch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A test-quality review ran 41 mutations and 13 survived. The numerical core held (28 killed; the accuracy fixtures have teeth -- substituting the Simpson value fails all 24 parametrised cases), but the WIRING layer was close to untested, which is precisely what this file's docstring claims to protect. THE FLAG COULD BE MADE COMPLETELY INERT. Deleting the driver's one load-bearing line -- the assignment to factored_likelihood.TIME_QUADRATURE_DEFAULT -- left all four driver tests green, as did hardcoding 'simpson' there. The banner those tests inspect was built from `opts`, so it reported what was ASKED FOR rather than what was in force, and the in-process test set the module attribute ITSELF with a comment saying "exactly what the driver does" -- a hand-copy of the line, not the line. Nothing joined the two halves. The banner now prints the value read back OUT of the module, which is what makes it, and the tests asserting on it, load-bearing. THE UPPER EDGE GUARD WAS UNEXERCISED. Every fixture parked peaks near sample 0, so dropping the right-hand term returned +88.8 nats above truth -- the exact number the docstring cites as the reason the guard exists, in the evidence-inflating direction. An off-by-one in the same term also survived. phase_marginalization=True NEVER REACHED THE NEW PATH. Dropping the abs() entirely changed nothing any test could see, yet --distance-marginalization --phase-marginalization is the standard production call site. Every fixture used the affine helper, for which lnL(t) is ITSELF exactly band-limited -- an easier problem than production poses. Added coverage for both settings and for a nonlinear, -inf-off-table distmarg-shaped callback. THE MEMORY-CHUNKING PATH NEVER RAN. Dropping all but the first chunk was invisible: it needs 72 rows to trigger and the largest batch was 4. Production runs --n-chunk 10000, so every real call takes that branch. TWO DRIVER GUARDS WERE BREAKABLE. Making the refuse-guard fire unconditionally would reject EVERY ordinary ILE run, and the banner was free to claim "honoured: True" always; no driver test ever launched a default configuration. Also pinned: the one-ulp factor bump, the non-finite argmax mask, sigma_t_min in the report, and the tuned constants themselves (the suite otherwise pinned EDGE_GUARD_FRACTION only to within a factor of ~10 while the docstring justifies it with a measured table). Replaced a vacuous assertion -- unmeasurable rows can never also be exposed, since the guard is gated on has_peak -- with one that the five counters PARTITION the batch, so no row can fall through a gap and be invisible in a log. 72 tests; count guard raised by running collection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Re-ran the mutation sweep against the CURRENT tree rather than trusting the earlier one, whose sed anchors no longer matched anything -- a stale anchor reports as a survivor, indistinguishably from a real one, so that sweep was describing a revision that no longer exists. 15 mutations; the fixed harness asserts each anchor matches EXACTLY ONCE and verifies the pristine restore byte-for-byte between mutants. Two survivors, both defects in the tests I had just added to kill them: The banner test matched the wrong LINE. `--interpolate-time` prints the identical phrase "honoured by this configuration", so asserting that the substring "...: False" appears somewhere in the output passes as long as EITHER banner says False. A mutation making the quadrature banner claim True unconditionally survived on the stencil line's False. Now matched with an anchored regex that pins the quadrature line's value and its honoured flag together. The right-edge test was not at the edge. It parked peaks at samples 583-613, far enough inside that `jmax > npts-1-guard` and `jmax > npts-guard` agree; the off-by-one leaves exactly one row's worth of the guard band open. Now driven to the boundary indices themselves -- guard-1 / guard and npts-1-guard / npts-guard -- and it also asserts the accepted rows are actually REFINED there, so the guard is deciding something rather than being masked by a factor of 1. Documents a corner found while writing it: a peak on the very first or last SAMPLE classifies as flat rather than wrap-exposed, because the curvature stencil is clipped inward and measures a positive second difference. That understates the window-centring problem in the diagnostic but is safe -- such a row is never refined, so it gets the historical value either way. Pinned as such. Also a skip guard in the CI gate. `pytest -q` exits 0 with skips, so a test that quietly stops running reads as green, and the count guard catches DESELECTION not SKIPPING. Exactly one skip is expected on a CPU runner (the GPU parity test); zero on a GPU runner, where RIFT_CI_REQUIRE_GPU=1 makes it fail instead. Final: 73 tests, 15/15 mutations killed, gate green end to end. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e record
Fifth external review, three findings, all confirmed.
1. The label reached only the SAMPLE file. write_samples() early-returns when
--save-samples/--output-file is absent, and write_dat() then published the
numeric evidence row unchanged -- so a run with export disabled got neither
warning nor any persistent label, and the primary .dat could be collected as
an ordinary integration. There is now angle_grid_suspect_note(), write_dat
puts it in the .dat header, and analyze_one emits the stderr warning ONCE
per event BEFORE either artifact is written, independently of export flags.
2. jax.debug.callback cannot carry correctness-critical provenance: effects may
be dropped, duplicated or reordered under transformation and may land after
the result is ready -- so a reader can see CLEAN while a tripped callback is
in flight, or reset for the next event before the previous event's callback
arrives. amp_failsafe_state() and reset_amp_failsafe() now call
jax.effects_barrier() first, and the docstrings say plainly that this is a
best-effort LABEL, not a gate: a clean read is not proof of adequacy.
3. The callback was unconditional, so every ordinary likelihood evaluation
transferred to the host and mutated n_calls -- once per MALA/flowMC
proposal, per chain, destroying accelerator throughput even when undersizing
never occurs. It now sits inside lax.cond; only the rare tripped branch
pays anything.
Tests: the driver test now checks write_dat carries the note and analyze_one
reports per event; a new test pins that debug.callback is INSIDE lax.cond and
that both state accessors barrier. EXPECTED_TESTS 172 -> 173.
Not yet fixed, and the reason CI is still red: the gate run is being killed by
the runner ("received a shutdown signal", exit 143) at ~15 min, i.e. resource
exhaustion rather than a test failure -- the collection floor now matches
(172/172). m_max-scaled dense sizing enlarged the remaining gated cases; that
is the next thing to reduce.
… bites
Three CI failures, three different symptoms, one cause -- the suite is too
expensive for a per-PR gate on a 2-core runner:
* 169 tests: CANCELLED at the job's 60-minute timeout-minutes cap (65 min);
* next head: OOM-killed the runner at 19 min;
* next head: reached 83%, then "the runner has received a shutdown signal"
(exit 143) six minutes later with no further progress.
The 139-test baseline ran in 13m53s. The collection floor was never the
problem after the first fix -- 172/172 matched.
So test_angle_marg_exact.py is EXCLUDED, with its reason, its cost history and
a copy-pasteable hand-run command written next to the entry. It is a
development check in the same sense full RIFT analysis runs are: it establishes
the schemes' error law at production amplitude, which is a property of the
mathematics and does not change commit to commit.
What could NOT simply be excluded is the sizing regression. My earlier
justification for the split -- "exactness does not depend on amplitude" -- is
false for the dense quadrature, and every low-scale brute-force test in that
file PASSES under the old m_max-blind rule. Excluding the file wholesale would
therefore have made a revert of _dense_grid_sizes green. So
test_dense_phi_sizing_must_scale_with_m_max is extracted into its own gated
file, test_angle_marg_sizing_rule.py: pure numpy, closed-form I0 reference,
10.8 s including the RIFT import, and it FAILS under the broken rule (0.498
nats vs 1.17e-10). It carries a self-check that fails if it ever stops biting.
EXPECTED_TESTS 173 -> 141 (139 baseline + the extracted regression + parametric
siblings), counted by collection. Ledger entry rewritten to describe what is
actually gated rather than what used to be.
The first real end-to-end run died in the terminal stage: both per-model CIP
jobs failed with
IndexError: too many indices for array: array is 1-dimensional
because approx_<A>_all.net was EMPTY. The cause, from unify_model.sh's stderr:
unify_model.sh: line 2: macroapprox: command not found
ls: cannot access '.../approx__*.composite': No such file or directory
Inside bash, $(macroapprox) is COMMAND SUBSTITUTION. It runs a nonexistent
command, expands to the empty string, and the glob silently matches nothing.
Condor never sees it: macros are expanded in the SUBMIT file, not in the script
the submit file invokes.
This is the THIRD instance of the same bug in this builder, and the second I
wrote myself. The first was join_grids.sh globbing approx__overlap-grid-*,
which this PR's own design note describes at length. I documented the trap and
then walked into it in a file I added.
Fixed the same way join_grids.sh already worked: when the pattern carries a
macro, the script takes it as $1 and the .sub passes it as an argument, so
condor does the expansion. Applied to write_unify_sub_simple and write_cat_sub;
both default to the previous behaviour when no macro is present.
AND A GENERAL GATE, because two fixes and a design note did not stop a third
instance: test_no_condor_macro_survives_into_a_shell_script scans every emitted
.sh for $(macro...). It found catjob.sh on its first run -- a live bug not yet
hit at run time, where every model's cat job wrote
extrinsic_posterior_samples_$(macroapprox).dat, i.e. the SAME file
"..._.dat" for both models, silently overwriting each other's extrinsic
posterior. That is the output the whole workflow exists to produce.
The class is invisible to every check that existed: DAG shape is fine, the
directories exist, the macros in the .sub files all resolve. It only appears at
run time as a missing file, which is why the assertion is at build time.
23 tests pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The `_DENSE_CHUNK_BYTES` docstring claimed chunking "cannot change the answer", and the test asserted bit-identity between a chunked and an unchunked run. Both were too strong. The batch shape reaches numpy's FFT and its pairwise summation, so a differently-chunked run reassociates; the companion peak-local implementation inherited this same wording, had a bit-identity test written to it, and measured 0, 0 and 2 ULPs. This fixture happens to come out bit-identical, which is precisely why asserting it was a latent flake rather than a passing test: it would have held here and failed on a different row count or chunk boundary. The assertion is now a tolerance far below anything that could hide a real assembly bug -- dropping a chunk moves rows by nats, not ULPs -- and the docstring states what is actually guaranteed. Found by an independent reviewer of the peak-local work, who scoped it out of their diff and left it here where it belongs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…call sites
FOURTH P2 finding on this PR, and the third on the SAME predicate. It went
`flag` -> `flag + envelope` -> `flag + envelope + path`, each round adding a
conjunct the dispatch-time expression already had in effect, each round found by
a reviewer rather than by a test. It could not be otherwise: an over-broad
predicate downgrades the inherited stencil to 'nearest', which is EXACTLY the
historical behaviour, so nothing fails, nothing logs, and no value test
distinguishes it.
THE DUPLICATION IS THE DEFECT, so the fix is structural rather than a third
conjunct. New fused_calmarg_in_use(opts, calibration_marginalization=None) is
the single definition; both the startup stencil guard (_fused_calmarg_would_run)
and the dispatch-time use_fused_calmarg now call it. A fused kernel runs only
where all of these hold, and each is a REACHABILITY fact checked against the
call sites, not a guess:
* calibration marginalization configured;
* --calibration-fused-kernel (opt-in);
* --calibration-n-realizations > 1 -- factored_likelihood returns from its
`n_cal == 1` branch before cal_method is read at all;
* NOT --rotation-slow and NOT --freqresponse -- both REPLACE the likelihood:
the non-distmarg fused call site sits in the `else` of their dispatch, and
the two distmarg sites are unreachable because both options refuse distance
marginalization at startup;
* NOT --calibration-dump-responsibilities -- that pilot evaluates with an
explicit cal_method='loop' and return_cal_components=True (which the library
also refuses to fuse) and RETURNS before production integration is built.
The last one matters beyond this PR: the pilot is a legitimate diagnostic mode,
not an incompatible combination, so no rejection layer should ever remove it and
this predicate has to keep excluding it on its own. The rotation-slow and
freqresponse conjuncts stay correct whether or not those combinations are
rejected earlier elsewhere -- rejection makes them unreachable, not wrong.
WHAT THE EARLY CALL CANNOT SEE, stated instead of left to be rediscovered. At
startup `calibration_marginalization` does not exist yet; it is set several
hundred lines later by `if opts.calibration_envelope_directory:` from a False
initial value, so passing None substitutes that option -- a restatement of the
same condition, not an approximation. It is also CHECKED: the dispatch-time
call passes the real variable and the driver REFUSES if the two disagree, so a
change to how calibration_marginalization is derived fails loudly instead of
silently re-opening the drift. Nothing else in the predicate is unavailable at
startup; every other term is a command-line option.
TESTS PIN THE STRUCTURE, NOT A VALUE, because the next forgotten conjunct will
be a fourth expression that agrees with every case anyone thought to write down:
test_the_fused_predicate_has_ONE_definition_shared_by_BOTH_call_sites
-- one FunctionDef, >=2 call sites, neither old hand-written shape back in
the file, and the startup/dispatch agreement tripwire still present.
test_no_fused_configuration_that_cannot_fuse_downgrades_the_default
-- the five negative cases, one per REASON rather than folded together,
since they fail differently and one expression has already been wrong
about three of them.
The positive edge is unchanged and still pinned: a genuine baseline fused run
still downgrades (test_default_stays_off_the_fused_calibration_kernel).
MUTATION-CHECKED, 8 mutants, 8 killed:
drop the rotation-slow / freqresponse / dump-responsibilities /
n-realizations conjunct (4) -> test_no_fused_configuration_that_cannot_fuse...
drop the envelope substitution -> 2 tests
predicate never true -> 3 tests
re-inline an early copy -> test_the_fused_predicate_has_ONE_definition...
delete the agreement tripwire -> initially SURVIVED (it cannot fire until
someone changes how calibration_marginalization is derived, so no
behaviour test can reach it); now killed by an ast check asserting the
tripwire is PRESENT. Labelled in the test as a structural pin, not
claimed as behaviour coverage -- an unexercisable guard that can be
silently deleted is worse than one that is honestly marked.
RELATED, FOUND WHILE SURVEYING THE OTHER CONSUMERS, NOT FIXED HERE. :535 and
:548 guard --rotation-slow / --freqresponse on GPU against calibration
marginalization using `getattr(opts, 'calibration_marginalization', False)`.
There is no --calibration-marginalization option and nothing ever assigns that
attribute, so BOTH guards are dead and have never fired; the live state is the
module-level calibration_marginalization, not set until much later, so the check
would have to read opts.calibration_envelope_directory -- the same substitution
fused_calmarg_in_use documents. Recorded in 9.6.3 and reported on the PR for
the separate early-rejection change, which covers exactly those combinations.
Gates: q-window-stencil-check list 64 passed, 2 skipped; the three companion
suites 115 passed, 1 skipped.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nt that lied External review, correct: the whole-cell fallback does not add nodes. It spreads the SAME n_nodes over a wider interval, so rejecting a stalled peak made the resolution WORSE, not safer -- and the comment I had written there claimed the branch "can only add nodes, never move the centre", which asserts the opposite of what the code does. That is the defect class this work keeps finding in other people's code, in mine. Reproduced by search rather than taken on report: a table whose fallback cell moves the inner integral by 1.7e-03 nats between 64 and 1024 nodes -- worse than the 7.2e-4 the reviewer measured. The node count is now DERIVED, not fixed: |d2g/du2| <= M2u = |c1| + 4|c2| exactly, so nothing on this axis is narrower than sigma_min = 1/sqrt(M2u), and a spacing of sigma_min/_PTS_PER_SIGMA resolves the sharpest feature the coefficients admit. That takes the counterexample from 1.7e-03 to 2.2e-04 nats at essentially no cost (2.2-6.4 s across the amplitude range, errors 0.0). WHAT WAS DELIBERATELY NOT DONE, with the number that decided it. A boundary-peaked fallback cell is not Gaussian there -- exp(g) falls off like exp(-|g'| du), so full convergence needs a spacing set by 1/M1u rather than 1/sqrt(M2u). Implemented and measured, that costs a 25x slowdown (2 s -> 49 s) to recover the remaining 2.2e-04 nats. Not taken. 2e-04 nats is orders below anything this rule is asked to decide -- its acceptance tolerance is 23 nats -- and the trade is written into the code with its measured price so the next reader can make it knowingly rather than rediscover it. Raising _PTS_PER_SIGMA or passing n_nodes are the honest knobs if it is ever wanted. Scope, checked rather than assumed: this path is reachable from nothing. grep finds no importer of phi_local_marginalize or u_profile outside their own module, so no production calculation -- 3g included -- can reach it. 26 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…marg_wiring Wire 'peak-local' into ILE as an angle-marg scheme (explicit-only, ready for a head-to-head pilot)
The drift ledger caught the new module-level function in the main ILE driver and refused it, which is the gate working: every function that appears in one driver and not the other has to carry a recorded decision about LISA, written as a RULE with a reason rather than hand-edited into the JSON. NA. Every term of this predicate is a LIGO/Virgo calibration concept -- an envelope directory, a realization count, --calibration-fused-kernel, the responsibilities pilot -- and the LISA driver has none of them: it declares no --calibration-* option, never sets cal_method, and models no instrument calibration at all (`grep -c calibration_fused_kernel|calibration_envelope_ directory|cal_method` on integrate_likelihood_extrinsic_batchmode_lisa is 0, 0, 0). Same reason as the existing --calibration-* rule this one sits beside. Called out in the rule because it is the one thing that could mislead a later reader: this function is READ BY THE STENCIL GUARD, and the LISA driver does have --interpolate-time, so it looks like a stencil gap. It is not. LISA's --interpolate-time is a separate BOOLEAN parsed by legacy_time_interpolation_enabled, with no fused kernel to protect, so there is nothing for this predicate to decide there. If LISA ever models calibration, the thing to port is the one-definition discipline, not this function. Ledger regenerated with the generator, not edited: 91/91 classified, NA=48 PORT=43, and the JSON diff is the four lines of the single new entry. test_lisa_driver_drift.py: 8 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eference Reviewer: "marker validation is fail-open -- any syntactically valid marker is treated as coverage." Correct, and the docstring made it worse by asserting the opposite: it claimed all three reachability signals could only UNDER-report, never falsely clear a file. That was false for two of the three, and both were live. 1. MARKER NAME UNVALIDATED. MARKER_RE matched shape, not meaning, so `# RIFT-CI-GATE: totally-made-up-job` -- which no script greps for -- cleared a file. Planting it on test/test_hlm.py, a file that cannot even be IMPORTED because it wants the pre-package `factored_likelihood`, moved it into the "reachable from CI" count and made its LEGACY roster entry report as STALE. That is the inert-guard failure this census exists to catch, reintroduced inside the census. Fix: KNOWN_GATES maps gate name -> the script that discovers files by that marker. A marker counts only if its name is registered AND that script really greps the literal. An unknown name is a hard error naming the typo, not a silent demotion to "unregistered", because the actionable diagnosis is "fix the marker", not "add a roster entry". KNOWN_GATES is a shared line -- one per JOB, edited when a gate is created rather than when a test is added, so it does not carry the per-test conflict cost that motivated PR #242's marker. 2. A NAME IN A COMMENT COUNTED AS A REFERENCE. A comment is exactly where a CI file explains what it does NOT run, so it is the one place a name appears without being invoked: ci.yml's comment saying why the two cupy parity files are EXCLUDED was enough to mark them covered. Worse, _cfg_blob() read .travis/*.py including this script, so a filename typed into an explanatory comment HERE marked that file covered -- which is how it was found, while writing the fix for (1). Fix: strip comment-only lines, and exclude this script from the blob. Every genuine reference is in a run: block, a pytest argument or a FILES array; none is on a '#' line. Consequence: the two GPU parity files are now correctly rostered rather than carried by a comment, which retires the apologetic note that stood in their place. The same hole existed one level down in the fix for (1): test-q-window-stencil.sh quotes its own marker inside the comment block explaining the mechanism, so a bare `literal in text` passed even with the live MARKER= assignment renamed away. Checked, it did. Both uses now go through _strip_comment_lines. Five new mutations, each broken and seen to fail; the original eight re-run and still fail. Table in the PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ocal DRAFT: phi-localization -- both angle axes localized, cost flat in amplitude
Review finding on #237 (P1): the two `getattr(opts, 'calibration_marginalization', False)` guards never fire, and gating on `opts.gpu` would leave the CPU replacement paths unprotected even if they did. Both halves are correct. This gate already removes those guards and refuses on the envelope alone, on either backend -- but nothing PINNED the backend-independence, so re-adding an `opts.gpu and` condition would have passed the whole suite. The two existing 3G tests both run through `_honoured(...)`, which sets gpu=True. Adding the CPU case is not a one-line flip: at gpu=False the configuration also earns the separate --gpu refusal, so `len(r) == 1` would then pass for the wrong reason. The new test asserts by MEMBERSHIP on (--calibration-envelope-directory, --rotation-slow / --freqresponse) instead. Mutation-tested rather than assumed. Reintroducing the reviewer's defect -- `if rotation_slow and xpy_evaluator:` and the same for freqresponse -- fails the new test and nothing else (1 failed, 23 passed); restored, 24 pass. So the test discriminates the backend condition specifically. Verified on rift_O4d while confirming the finding, since the refusal messages assert it: neither replacement entry point takes n_cal or cal_method (factored_likelihood_with_rotation.py:770, factored_likelihood_freqresponse.py:328), and factored_likelihood_freqresponse.py contains no calibration reference at all. Stronger still, the rotation kernel's own docstring (:778) already states "Requires n_cal=1 (no glitch/calibration marginalization)" -- a documented precondition whose only enforcement was the inert guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…NG an expiry
Three P2s, all correct. Each was reproduced before it was fixed.
[P2] MARKER VALIDATION DID NOT PROVE CONSUMPTION. Checking that the marker literal appears
on a non-comment line passes an ORPHANED `MARKER=` assignment whose discovery grep has been
deleted. Tightening it to "and a line matching grep.*MARKER" does not fix it either:
test-q-window-stencil.sh contains `grep -qxF -- "${MARKER}" "${e}"` for the opposite purpose
-- asserting an EXCLUDED file does NOT carry the marker -- so deleting the real discovery
line still satisfied the pattern. Tried, and it passed green. No regex separates "uses the
marker to find files" from "uses the marker".
So the inference is gone. A gate is honoured only if its script can be ASKED:
RIFT_CI_GATE_LIST=1 must print the files it would run and exit 0, and that listing is what
the census believes -- a file carrying a valid marker that the gate does not return is now an
error against the file. Placement is part of the contract (discovery above the dependency
probes) because this job has no `needs: install`. A marker naming a gate that cannot list is
an error quoting the snippet; a registered gate with no marked files costs nothing.
[P2] DIRECTORY COVERAGE SURVIVED JOB REMOVAL. DIR_TARGETS checked only that the directory
existed. Deleting the asimov-integration job from ci.yml AND .gitlab-ci.yml left
.travis/test-asimov.sh on disk, still naming the directory, and the census went on reporting
its three files as covered -- green, with nothing running them.
Root cause was wider than that entry: the config blob was "every file in .travis/", so ANY
script no job invokes still conferred coverage. It is now the transitive closure from the
workflow entry points -- a script joins when something already live names it -- so deleting a
job drops its script out and its files go red. Removing the lisa job now flags 14 files.
DIR_TARGETS additionally requires the directory to appear in that live set.
[P2] PENDING NEVER EXPIRED. Its exemption from the stale check was unconditional, which
bought merge-order independence at the price of the one status nothing could ever force out.
It now carries a condition that can be evaluated: the reason must name `gate:<name>`, and the
entry is legal only while that gate is NOT live. When the gate lands, the entry is an error
naming itself. The cost -- merging the companion PR needs a one-line deletion here -- is a
forcing function, and is stated rather than engineered away.
Verified against a real merge with the q-window-stencil branch: merges clean, and with an
8-line hoist there (discovery above the probes plus the list-mode line) the census passes and
that gate still reports 8 registered files / 46 collected / 44 passed. Patch offered on #242.
14 mutations, each broken and seen to fail, plus the honoured path checked positively.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tencil-manifest ci: stop q-window-stencil-check silently losing tests (in-file marker + fail-closed manifest)
#242 is merged, so the q-window-stencil gate now exists on rift_O4d and the census's requirements against it are live rather than hypothetical. All three edits predicted in the PR body, and nothing else: 1. .travis/test-q-window-stencil.sh: hoist CODEDIR + the marker discovery above the pytest/numpy/lal probes, and answer RIFT_CI_GATE_LIST=1 with the discovered files. Discovery needs only grep, so nothing is lost by running it first, and ci-roster-check -- which has no `needs: install` -- can now ASK this gate what it runs instead of pattern- matching its source. That distinction is not pedantic: this script contains both the mapfile that discovers files AND a `grep -qxF -- "${MARKER}"` that asserts an EXCLUDED file does NOT carry the marker, so any regex-based check keeps passing with the discovery deleted. Verified: the gate still reports 8 registered files, 46 collected, 44 passed. 2. The two cupy parity legs leave the roster. #242's EXCLUDED array records them with the same reason plus its own fail-closed existence check, which is a better home -- the decision now sits beside the gate it belongs to. 3. The PENDING entry expires, exactly as designed: its gate went live, the census named the entry, and the entry is gone. The status keeps its definition and its tests; it simply has no users. Its header text is corrected too -- it still described the old unconditional "legal in either merge order" semantics, which stopped being true when the expiry landed. Census on the merged tree: 201 test files, 147 reachable, 54 rostered, PASS. The merge also brought new tests from #235 and #239; all are already registered, which is the census doing its job quietly. core-unit-check unchanged at 278/266/12. Eight mutations re-run against the now-live gate, including four that only became testable once it existed: list mode removed, discovery deleted, listing narrowed to drop a marked file, and the gate script left in place but invoked by no job. All fail; the honoured path passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…test with it The conflict was in .github/workflows/ci.yml and it is the conflict #242 was written to abolish: this branch hand-added test_batchmode_stencil_default.py to the shared pytest list, and #242 replaced that list with .travis/test-q-window-stencil.sh, which discovers members from a marker line inside each test file. Resolved to the script. BUT RESOLVING IT THERE WOULD HAVE SILENTLY DROPPED THIS PR'S TEST. The script discovers by marker, and the new file carried none -- it was only ever registered by the ci.yml line the merge deleted. So the merge as `--theirs` alone builds green with 23 tests no longer running. Marker added to the test file. AND THE FAIL-CLOSED MANIFEST WOULD NOT HAVE CAUGHT IT, which is the part worth fixing rather than working around. The manifest fails the job for any file matching SCOPE_GLOBS that carries no marker -- but the globs owned test_q_window_*, test_time_interp_*, test_interpolate_time_*, test_calmarg_stencil_* and test_noloop_*, and this file is test_batchmode_stencil_*. It fell in the gap the script's own header admits ("a new stencil test filed under some OTHER prefix"). Added test_batchmode_stencil_*.py to SCOPE_GLOBS, so the next one is caught rather than depending on whoever resolves the merge noticing. Mutation-tested, not assumed: with the marker stripped the job now FAILS with "neither registered nor explicitly excluded" and prints the line to add; restored, it passes. Before the glob was added the same mutation was green and silent. Floors re-derived as the script's header requires: EXPECTED_TESTS 46 -> 69, EXPECTED_PASSED 44 -> 67 (the new file collects 23). MAX_SKIPS unchanged at 2 -- the two cupy legs. Raised, not lowered. Verified on citlogin6 with the IGWN CVMFS python: 9 registered files, 69 collected, 67 passed, 2 skipped, rc=0. A first attempt on the session host died mid-run at 454/500 threads and reported success from the launcher rather than the script -- the run above is the real one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI caught what my local run could not: 69 collected as pinned, but 3 skips against the cap of 2, so 66 passed against a floor of 67. The gate did exactly its job -- a skip is how a gate gets disabled quietly, which is why it is capped rather than tolerated. THE SKIPPED TEST WAS test_the_two_ile_drivers_ship_the_same_default -- issue #233's single assertion, the one tying the batchmode and jax defaults together. It imported RIFT.likelihood.jax_ile.core, and q-window-stencil-check runs on a numpy+lal image with no jaxlib BY DESIGN ("Needs numpy + lal only", the script's own header). So the assertion was skipped in the only job that runs this file: a gate skipped exactly where it is needed is not a gate. It passed on citlogin6 because the IGWN conda env has jax, which is why I did not see it. FIXED BY REMOVING THE SKIP, NOT BY RAISING THE CAP. The script does allow "raise MAX_SKIPS and say which test and why", and I could have justified it -- the docstring already argued the skip was benign because core.py ALIASES the shared default rather than copying it, so drift is structurally impossible. That argument is true and it is still the wrong fix: it accommodates a hole the docstring itself calls "a real hole". The test now reads the binding out of jax_ile/core.py with ast and never imports the module, so it runs everywhere. That is also a STRONGER assertion than the one it replaces: the old version compared two values, so two independently written literals both reading 'sinc' would have satisfied it -- and two independently written literals are exactly how the drivers came to ship opposite defaults. The new one requires JAX_INTERP_DEFAULT = TIME_INTERP_DEFAULT as a NAME binding. Mutation-tested: replacing the alias with a literal of the SAME value ('sinc') fails the new test and would have passed the old one. Restored, it passes. Floors therefore unchanged at 69 / 67 / 2 -- nothing lowered. Full gate on citlogin6: 9 files, 69 collected, 67 passed, 2 skipped (the two cupy legs), rc=0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… > 1 Review P2 on #244. Verified against the source rather than the flag names, and the reviewer's request to "audit the other options guarded by n_cal > 1" found a second one. --calibration-fused-kernel at n_cal == 1. factored_likelihood's `if n_cal == 1:` branch RETURNS (:2942) before `cal_method == 'fused'` is read (:3041), so the fused reduction is unreachable with one realization. The run is not wrong -- the ordinary reduction is correct -- but it advertises a kernel it did not use, which is this gate's whole subject. --calibration-burn-in-neff at n_cal == 1, which the P2 did not name. The driver's burn-in block is `if opts.calibration_burn_in_neff and calibration_marginalization and n_cal_for_likelihood > 1:` (:4058), so one realization means no burn-in and a silently ignored target. Same prerequisite, same silence. NOT INCLUDED, deliberately: --calibration-conjugate-phase and --calibration-global-norm. They reach the likelihood through extra_kwargs on the PRECOMPUTE and are honoured at n_cal == 1 too -- that branch uses rho_sq_cal[0] -- so refusing them would be a false positive, which is the failure this gate exists to avoid rather than commit. Pinned by a test that asserts they stay accepted. n_realizations is passed in but stays OUT of CAL_OPT_IN_FLAGS: it has a non-None default (100), so a rule over its presence would refuse every calmarg run. A test pins that the new rules are silent when the default is left alone. MUTATION SWEEP, three mutants, all killed: disabling the predicate, an off-by-one (`< 1` for `<= 1`), and an over-broad version that fires without the flag. METHOD NOTE, because it nearly cost a wrong conclusion: the over-broad mutant first appeared to SURVIVE with 31 passed. It had not survived -- the test host reads this tree over NFS and was still serving the pre-mutation file. Re-run with md5sum compared on both sides, it fails at test_option_compat.py:314 as intended. A mutation that "survives" because the runner never saw it is a false negative that reads exactly like a coverage gap, so every mutant here was confirmed with the remote md5 matching the local one. 31 tests pass on the IGWN CVMFS python. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ion_gate ILE batchmode: refuse calibration-option combinations that cannot work (calmarg x 3G is untested, so it must not silently degrade)
…tencil_default batchmode: default the Q_lm sub-sample stencil to sinc, and downgrade rather than refuse (result-changing)
…kout
[P2] PENDING REMAINED STALE WHEN ANOTHER JOB REGISTERED THE FILE. Correct. The PENDING branch
checked its named gate and then `continue`d UNCONDITIONALLY, skipping the staleness check
below. So a file that became reachable through some OTHER job -- while the gate it named
stayed dormant -- kept its PENDING entry for ever. That is the same "never expires" defect the
previous round removed the blanket exemption to close; it survived one level in.
PENDING now carries an EXTRA condition rather than a weaker one: it falls through, so it goes
stale the moment the file is covered BY ANY JOB, and separately when its gate goes live. The
message says which, because "delete this line" for the two reasons has different follow-up.
Reproduced both ways before landing: a reachable file rostered PENDING on a dormant gate now
FAILS ("listed as PENDING but IS now reachable"), and with the `continue` restored the same
roster passes green. The legitimate case -- unreachable file, gate not yet landed -- still
passes.
TEST-CALMARG.SH TESTED WHATEVER WAS INSTALLED. Its first check runs
`python <path>/test_precompute_alignment.py`, which puts the SCRIPT'S directory on sys.path and
not Code/, so bare `import RIFT` resolved to the installed package. In CI that is the editable
install of this checkout, so it passes and the gap is invisible; on a plain CIT checkout it
picked up the CVMFS IGWN RIFT, which predates a kwarg the checkout added, and reported that
staleness as a failure of this branch:
PrecomputeLikelihoodTerms() got an unexpected keyword argument 'calibration_realizations'
I nearly filed that as a #237 x #244 interaction. One `export PYTHONPATH` prepend at the top
fixes the whole script and matches the invariant its sibling gates state explicitly. The
`python -m RIFT.calmarg.*` runs were already safe (cwd is on sys.path under -m); this makes the
rest safe the same way. Verified: 31 passed on a bare checkout, where it previously died at
import.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r-census CI: census every test file against a roster, and gate the 31 that were running nowhere
oshaughnessy-junior
deployed
to
private-review-dispatch-rift-upstream
September 4, 2026 11:30 — with 
GitHub Actions
Active
GitHub Actions
Active
…gration Exercise Rimsky through Asimov submission
oshaughn
deployed
to
private-review-dispatch-rift-upstream
September 4, 2026 12:49 — with
GitHub Actions
Active
GitHub Actions
Active
oshaughn
deployed
to
private-review-dispatch-rift-upstream
September 4, 2026 13:42 — 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
2 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.
Summary
Sync upstream
rift_O4dwith the 44 PRs merged into the junior fork since the shared base:Integration notes
git merge-tree --write-treecheck completed without conflicts.