Propagate tidal parameters through GWSignal - #180
Open
oshaughnessy-junior wants to merge 1 commit into
Open
oshaughnessy-junior wants to merge 1 commit into
oshaughnessy-junior wants to merge 1 commit into
Conversation
oshaughnessy-junior
deployed
to
private-review-dispatch-rift-upstream
August 20, 2026 17:49 — with 
GitHub Actions
Active
GitHub Actions
Active
oshaughnessy-junior
marked this pull request as ready for review
August 24, 2026 19:46
oshaughnessy-junior
deployed
to
private-review-dispatch-rift-upstream
August 24, 2026 19:46 — with
GitHub Actions
Active
GitHub Actions
Active
![interhost-cross-review-canary-v2[bot]](https://avatars.githubusercontent.com/u/274802661?s=60&v=4){kind=small}
oshaughn
pushed a commit
that referenced
this pull request
Sep 4, 2026
…rom the SNR RO'S (2026-08-23): the non-JAX helper picks this exponent with intelligent historical choices; the JAX protocol has no such intelligence and should have something. This is that something -- but keyed on a different variable, because measuring first showed the historical rule does not transfer. WHAT THE HISTORICAL RULE IS. helper_LDG_Events.py:1472-1481 sets beta = 0.1 for SNR <= 22.5 and 0.1*(22.5/SNR)^2 above; equivalently beta = min(0.1, 25.31/lnLmax), i.e. it holds beta*lnLmax fixed at ~25.3 nats. util_RIFT_pseudo_pipe.py does not set it; the helper is the only chooser. WHY IT DOES NOT TRANSFER. In every non-JAX consumer the exponent shapes only the adaptive sampling PRIOR -- mcsamplerGPU keeps log_integrand = lnL + ln p - ln p_s while tempering only log_weights -- so the estimator is unbiased at any beta and the exported sample count is untouched. On the JAX flowMC path beta = inv_T is the exponent of the target the MCMC SAMPLES, the draws are the deliverable, and the export must be reweighted by L^(1-beta) (PR #180). That reweight costs ESS/N = Z_1^2/(Z_beta Z_{2-beta}) = [beta (2-beta)]^(dim/2) which is set by the SAMPLED DIMENSION and carries no lnLmax term at all. MEASURED, two independent routes (DESIGN_jax_tempering.md): * exact offline sweep on the real BNS likelihood (SNR 23.8, 4-D), defensive-IS reference at ESS 5235 with every exponent in [0,2] resolved: the law holds to 0.79-1.00 over beta in [0.05,1]. * the driver's own reported export ESS, which shares no machinery with the above: predicted 157 vs measured 185 at beta=0.0951, and predicted 4320 vs measured 4203 at beta=0.7735. * SNR ladder at fixed beta (injected distance): beta=0.5 gives ESS/N 0.620 at SNR~15 and 0.517 at SNR~33 -- a 17% drift where the historical rule would have demanded beta fall 4.7x. CONSEQUENCE. Porting the historical exponent literally costs 17x in exported rows (4800 -> 278) and 6.6x in evidence precision on the study event, and the driver already prints "NOT a usable posterior sample" over the result. WHAT THIS ADDS. * samplers.export_ess_fraction / beta_for_export_ess -- the law and its inverse. The signature takes no SNR, deliberately; a test pins that. * --auto-adapt-weight-exponent + --target-export-ess-frac (default 0.9): the smallest beta meeting the export budget, keyed on dim (3 -> 0.740, 4 -> 0.773, 5 -> 0.797). * a guard that REFUSES any beta whose predicted export ESS is below the driver's own usability floor, naming the historical rule as the trap. --allow-degenerate-tempering overrides. That floor is now one constant (_USABLE_EXPORT_ESS) shared with the warning fairdraw_indices already printed, so the two cannot drift. * conflicts raise rather than silently win: --auto with an explicit exponent, and --auto with --adapt-adapt. * --interp cubic is selectable (the gatherer is implemented and registered in _GATHERERS; the CLI could not reach it). Needed to measure the configuration the reference run actually used. DELIBERATELY NOT DONE: porting the SNR rule to the static exponent, and turning --adapt-adapt on by default. --adapt-adapt is an annealing SCHEDULE that always terminates at inv_T = 1, so it delivers the historical rule's benefit at zero reweight cost -- which makes it the right answer to the high-SNR problem and the wrong thing to call an exponent chooser. Whether it should be the default needs the high-SNR bake-off in PR #183. Stacked on claude/jax-fairdraw-extrinsic (PR #180): without that fair draw a non-unit exponent is not correctly handled at export, so this must not land first. Limitations, including the axes NOT swept, are in the DESIGN doc rather than implied by silence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
oshaughn
pushed a commit
that referenced
this pull request
Sep 4, 2026
oshaughn
pushed a commit
that referenced
this pull request
Sep 4, 2026
oshaughn
pushed a commit
that referenced
this pull request
Sep 4, 2026
…f them blocking
Found by running the repo's own RIFT/integrators/REVIEW_CHECKLIST.md against the
change. None was visible to the test suite or the CI gate, which were green over
all of them.
1. BLOCKING -- --auto killed every multi-event batch. resolve_tempering_exponent
wrote its answer back to opts. opts is per-RUN; analyze_one is per-EVENT. On
event 1 the chooser read its own event-0 output as a user-supplied exponent and
aborted with SystemExit. ILE_extr.sub runs batches, so every real multi-event
--auto run would have died. NO SINGLE-EVENT TEST CAN SEE THIS and every
measurement backing this PR is single-event; the checklist names the shape
("Is per-point state cleared on ENTRY"). Reproduced with
--n-events-to-analyze 3. The chooser is now PURE -- it returns the exponent
and analyze_one keeps it in a local.
2. The guard falsely REFUSED --smc-puffball. That flag routes these modes to
samplers.smc_puffball_sample, which swallows `temper` in **_ignore and returns
post_weight uniform with temper=1.0 -- the exponent has no effect there, so
refusing a run over it is a false alarm. The path now returns beta=1 and
REPORTS the no-op rather than leaving it silent.
3. --fisher-is-samples: a SUCCESSFUL Fisher-IS pass replaces the cloud with an
already-fair-drawn uniform-weight set, so the reweight cost never materialises
-- but it falls back to the tempered draws when it fails. The guard still
refuses (fail-closed); that is now stated in the message and the limitations
instead of being a surprise.
4. .github/workflows/ci.yml still documented EXPECTED_TESTS=27 and quoted its cost
and trim guidance against 27 tests. The count went 27 -> 48 (#180) -> 79 here.
Updated, with the runner-vs-local ratio spelled out so the 60-minute timeout is
justified rather than assumed.
Also: TWO OF MY OWN TESTS PINNED THE BUGGY CONTRACT -- they asserted the chooser
must ASSIGN opts.adapt_weight_exponent. Retargeted to the return-value contract
rather than deleted, so what they were actually protecting (that the chooser is
not a dead knob) survives.
Three regression tests added, each mutation-tested by re-introducing the exact
defect: writing opts again (lethal, 4 tests), the call site ignoring the return
value (lethal, 2 tests), and the smc-puffball path.
31 tests (from 28); EXPECTED_TESTS 79.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
oshaughn
pushed a commit
that referenced
this pull request
Sep 4, 2026
oshaughn
pushed a commit
that referenced
this pull request
Sep 4, 2026
oshaughn
pushed a commit
that referenced
this pull request
Sep 4, 2026
Review finding on #180. fairdraw_size clamped the requested export count by 1.5*res["neff"]. On every flowMC mode `theta` is the production posterior / tempered chain, while res["neff"] comes from a SEPARATE estimator: the moment-matched Gaussian importance cloud built for the evidence, or -- under --adapt-adapt -- the annealing ladder's minimum inter-stage ESS. Neither describes the exported rows. A valid equal-weight chain was therefore truncated whenever that unrelated proposal happened to have low ESS. Demonstrated on identical input (5000 equal-weight rows, --n-fairdraw-extrinsic-samples 1000): evidence neff = 2.0 -> 3 rows exported evidence neff = 40.0 -> 60 rows evidence neff = 5861.9 -> 1000 rows after this change -> 1000 rows, always Real runs of this branch report evidence neff of 5861.9 (beta=1), 163.8 (beta=0.0951) and as low as 4.1 for a poorly converged evidence proposal, so the truncating regime is reachable, and --fairdraw-extrinsic-output is in ILE_extr.sub. WHY THE ESS TERM IS REMOVED RATHER THAN REPOINTED. ILE's own clamp (mcsampler.integrate:802, n_extr = min(n_extr, 1.5*eff_samp, 1.5*neff)) is self-consistent because eff_samp, neff and the fair draw are all properties of the SAME weight vector `wt` the draw samples from. The port lost that. The ESS that does bound this export is the one fairdraw_indices computes from the EXPORT weights -- and it has already applied it, as n_out = min(ceil(1.5*ESS), n_in). So `n_have` already carries the 1.5*ESS cap wherever the weights were non-uniform, and where they were uniform there is no computed chain ESS to clamp by (an autocorrelation-based one would be a different, unimplemented quantity). A second clamp is a no-op where it would be right and wrong where it binds. The `neff` parameter is removed from write_samples and fairdraw_size entirely, so the evidence number has no path into the export count and the defect cannot be re-wired. WHY NOTHING CAUGHT IT: all 20 write_samples call sites in the test file passed neff=inf or neff=nan, so the clamp was never once exercised. Two tests added -- one behavioural (ask for 1000 equal-weight rows, require 1000), one structural (the parameter is gone from both signatures and from the analyze_one call site). Also corrects a pre-existing off-by-one in the gate's bookkeeping, found while updating it: test_jax_fairdraw_export.py collected 30, not the 29 the manifest documented, and the suite collected 57 against EXPECTED_TESTS=56. The floor is a >= test so it passed; the documented counts were simply wrong. Now 32 and 59, both verified by collection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
oshaughn
pushed a commit
that referenced
this pull request
Sep 4, 2026
…extrinsic jax ILE: make --save-samples a fair draw, not the raw sampler cloud
oshaughn
pushed a commit
that referenced
this pull request
Sep 4, 2026
…ubic) #180 has landed, so this PR now targets rift_O4d directly rather than stacking. Three conflicts: * --interp cubic. #190 shipped the same fix independently, with a help string explaining that linear undershoots the rholm peak and biases the recovered arrival time. Took THEIRS wholesale -- it is a strict superset of the bare `choices` widening this branch carried -- and dropped mine. * The gate's per-file ledger. Both entries are needed (this branch's chooser file, #190's test_interp_choices.py); kept both. * EXPECTED_TESTS. Resolved to 95 by RUNNING collection on the merged FILES list. 64 (rift_O4d) + 31 (chooser) = 95 confirms nothing else shifted. TWO STALE CLAIMS OF MY OWN, found by reading what landed rather than only the conflict hunks: * The gate ledger entry for test_jax_tempering_chooser.py still said the chooser is "actually assigned to opts.adapt_weight_exponent" -- the contract REMOVED in cf02b9d when that write turned out to make event 1 of a batch read event 0's choice. Corrected to the return-value contract. * The --smc-puffball branch comment said smc_puffball_sample "returns post_weight uniform with temper=1.0". #180 changed that: it now reports the inv_T its ladder actually reached and a matching L^(1-inv_T) weight, and its export weights are uniform only because analyze_one REFUSES the event unless the ladder finishes at inv_T=1. Skipping the guard is still correct -- the signature still swallows `temper` in **_ignore, so the exponent is genuinely inert -- but the comment now says why, and warns against re-deriving "its weights are always uniform" from that branch. Checked the driver's automerge kept both sides rather than trusting it: #180's 2-arg fairdraw_size and both length-mismatch guards are present alongside this branch's resolve_tempering_exponent, the resolved_beta wiring at both sampler call sites, and a single _USABLE_EXPORT_ESS shared with fairdraw_indices. 65 tests pass across the two interacting files (31 chooser + 34 export). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
oshaughn
pushed a commit
that referenced
this pull request
Sep 4, 2026
Caught reading PR #186's own diff after the rift_O4d merge. I wrote "79" when 79 was current; merging #180 (64) plus this branch's chooser took the floor to 95 and left my own note stale -- the same failure the note itself documents, one revision later. Also records where the count came from (27 -> 48 -> 64 via #180 -> 95 here, with #190's test_interp_choices.py along the way) and re-measures the wall time on the host it was actually run on (859 s, ldas-pcdev11, 16 cores), so the 60-minute timeout is justified against a current number rather than a stale one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
oshaughn
pushed a commit
that referenced
this pull request
Sep 4, 2026
…han assumed)
No conflicts this time. A clean automerge is not evidence the semantics
survived, though -- both sides had touched .travis/test-jax.sh and ci.yml -- so
checked explicitly:
* this branch: resolve_tempering_exponent, the resolved_beta wiring at both
sampler call sites, and the --smc-puffball no-op notice are all present.
* upstream: #180's 2-arg fairdraw_size and length-mismatch guard, and
TIME_INTERP_CHOICES from the stencil-validation work, are all present.
* manifest: 12 gated files + 2 explicitly excluded, every test_*.py in
test/jax accounted for, and collection returns exactly 95 -- so
EXPECTED_TESTS=95 and the ci.yml cost note both remain correct rather than
silently becoming a floor that no longer describes the suite.
Nothing to change; recording the checks because "the merge was clean" has not
been a sufficient answer at any point in this branch's history.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
This intentionally does not special-case string approximants or impose a mode ceiling; those behaviors remain with GWSignal and downstream RIFT handling.
Validation