_rvs fair-draw audit: enumerate every consumer, fix what it found, gate the rest - #87
Conversation
GitHub Actions
Active
![interhost-cross-review-canary-v2[bot]](https://avatars.githubusercontent.com/u/274802661?s=60&v=4){kind=small}
…te the rest sampler._rvs is not the sample set. integrate_log rebinds every key to a subset drawn WITH REPLACEMENT proportional to weight, of size min(n_extr, 1.5*eff_samp, 1.5*neff) -- an EXPORT resample. Five defects had come from consumers reading it as the sample set, each found only because a change happened to touch it. This is the mechanical sweep that was left open. THE CENSUS. 306 reads, 131 of which see the resample. audit_rvs_fairdraw.py enumerates and classifies them; --check runs in CI and fails on any post-rebind read without a recorded verdict, keyed by a hash of the read itself rather than its line or its function (analyze_event alone holds 40, and every known defect was added NEXT TO correct code). Ledger BROKEN count: 8 -> 1. Two results worth keeping: inside the integrators nothing post-rebind is broken -- all 26 lexically-AFTER sites are the rebind's own RHS or the cupy->numpy conversion loop -- and the production pipelines pass --fairdraw-extrinsic-output unconditionally, so this is never exotic. FIXED, sequential warm-start seed. --sampler-sequential-warmstart read _rvs and guarded on a row COUNT: PR #78's defect one code path away. Both directions are quiet -- on the measured pass the draw leaves ONE row so the count declines and the feature is silently inert; at ~5 rows it accepts a rank-2-of-6 seed and the next point warm-starts into a sliver reporting a healthy n_eff. Now seeds from the retained reserve and judges by affine RANK via build_warm_seed. The L0 rescue's inline reserve lookup is replaced by the shared _warm_seed_reserve_for so the pair cannot drift. FIXED, three double-weighting sites. A fair-drawn record is already equal-weight; weighting it by w again gives w^2. _pool_replica_rvs has guarded against this since the replica work; these had no equivalent: the --extrinsic-proposal-output GMM breadcrumb (worst -- its over-concentrated proposal is handed to the NEXT iteration, so the truncation compounds), .dgrid, and the .dslice reweight core. The last is a different shape -- it double-counts pi_Omega/q_Omega and takes N from the resample, and cannot be corrected once the pre-draw record is gone -- so it is routed to the exact all-fresh path, loudly, since that changes cost. The predicate is NOT the CLI flag: the samplers skip the draw when it would not shrink the record, and those rows still carry real weights, so flattening them would be the same error reversed. All seven rebind sites now mark the rebind itself (self._rvs_is_fairdraw, reset per pass since samplers are reused across events). ln_weights_for_posterior answers "how should these rows represent the posterior", distinct from ln_weights_from_rvs, which answers "what is this record's importance weight" and was always right about that. FIXED, pooled n_eff. Kish of the pooled record overwrites the reported neff, but on a fair-drawn export _pool_replica_rvs has flattened each block, and Kish of piecewise-constant weights is just the row count -- 5K at the default --fairdraw-extrinsic-output-n-max 5. Now Kish over the BLOCKS, (sum Z_k)^2 / sum(Z_k^2/neff_k), which reduces to sum(neff_k) when the replicas agree and falls below it when they do not: the property the original comment asked for. Pooled Kish still applies when the export was not fair-drawn. MEASURED, --sampler-l0-rescue-reject-dlnZ: default raised 0.5 -> 3.0. 160 known-lnZ passes across AV and portfolio. The gate caught 0 of 55 genuinely truncated warm passes at EVERY threshold from 0.25 to 4.0; at 0.5 it binned 25% of GOOD portfolio warm passes. 0.5 was strictly dominated. Rates are conditioned on what the warm pass ACTUALLY did, not the intended condition -- a portfolio's defensive GMM component means a seeded warm pass usually still reaches every mode (77 of 80), so scoring against the label would have counted false positives as successes. On AV the gate's premise is false outright: the collapsed cold pass reports lnZ 70-92 nats BELOW truth while the warm pass is within a nat. Not a helper bug -- lnZ_from_reserve reproduces integrate_log's own lnZ to the digit. Pre-#79 the +log(n/eff_samp) inflation occasionally pushed the difference positive, which is where the observed 10-of-12 rejections came from: the artifact, not the mechanism. Support containment is the recommended replacement and is deliberately NOT attempted here -- a new detector wants its own validation. Tests: 34 new across test_seq_warmstart_seed.py and test_fairdraw_double_weighting.py, both added to CI, plus the --check gate. 197 passed, 3 skipped across the integrator suites. Measurement scripts, trimmed run logs and a per-replicate JSON dumper are committed so every number above can be reproduced without re-running the passes.
…eserve survived rejection Both P1, both the same shape, and it is the shape worth naming: a fix that is correct in isolation and wrong once another code path runs after it. Every unit test from the original change passed with both bugs present, because each tested its helper rather than the composition. 1. THE FAIR-DRAW MARKER SURVIVED _pool_replica_rvs. The pooled record is equal-weight WITHIN each block but weighted BETWEEN blocks by exactly the replica evidences Z_k/K. With the marker still set, ln_weights_for_posterior returned zeros, so .dgrid and the proposal breadcrumb would have mixed replicas by exported ROW COUNT instead of by evidence -- discarding the disagreement the replicas are run to measure. The marker is now cleared after pooling, and ONLY when pooling actually happened: every fallback path in _pool_replica_rvs returns one of its INPUT records (too few replicas, no sampling-prior column, an exception), and such a record is still the fair draw it arrived as. Object identity, not length, is the reliable test for that. 2. A REJECTED WARM RESCUE LEFT ITS RESERVE BEHIND. The reject path restored _rvs, the estimate and dict_return, but not _warm_seed_reserve, which still described the rejected warm pass. Note the direction: this was latent until THIS BRANCH made the sequential warm start read the reserve. Before that the capture read _rvs, which the reject path does restore -- so the sequential fix is what activated it, and the next intrinsic point would have been seeded from the very truncated cloud the evidence gate had just thrown away. Snapshot and restore now travel together through _snapshot_pass_state / _restore_pass_state, carrying the reserve, the fair-draw marker and the PER-MEMBER reserves -- _warm_seed_reserve_for falls through to portfolio_realizations, so restoring only the aggregate would leave that fallback pointing at the warm pass. Both restore sites, the reject path and the exception handler, go through the one helper so they cannot drift in what they put back. On the tests, because it is a real limitation: analyze_event needs data, PSDs and a waveform, so the call sites cannot be driven from a unit test. The behavioural tests pin the contracts the call sites depend on; the wiring is pinned at source level, including a guard that the reject branch does not assign _rvs directly again. Verified by reverting each fix -- only the wiring tests fail, which is stated in the suite rather than left for the next reader to discover. Rebased onto rift_O4d (picks up #88 and #66); no conflicts. 7 new tests, 28 in the suite; 204 passed, 3 skipped across the integrator suites, and the ensemble script-style test still recovers AC/GMM/AV to ~1.0.
92c320f to
94f8c0e
Compare
GitHub Actions
Failure
|
Both P1s fixed, and rebased onto Both findings were correct, and they are the same shape — a fix that is correct in isolation 1. The marker survived
|
…r replica
Both P1s were correct, and the first is a defect I introduced answering the previous round.
ONE FLAG, TWO QUESTIONS. Clearing _rvs_is_fairdraw after pooling fixed the weighting question
and broke two others, because the flag carried two distinct properties:
rows were drawn proportional to w -- per BLOCK -- TRUE for a pooled record
the record is globally equal-weight -- whole RECORD -- FALSE for a pooled record
Clearing it stopped the .dslice all-fresh safeguard firing on a pooled record whose blocks ARE
resampled, and made the block-Kish n_eff branch UNREACHABLE: it sits below the line that
cleared the flag it tested, and is only ever reached when pooling happened. So it was dead
code from the moment I wrote it.
Now two predicates over two markers: _rvs_is_export_resample (rows resampled, survives pooling)
and _rvs_is_equal_weight (fairdraw and not pooled). The block-Kish branch keys on neither -- it
uses a local computed where pooling happens, because "did pooling flatten a block" is a fact
about that step, not a property of the record. _rvs_is_pooled is set by analyze_event rather
than by a sampler, so it is dropped with the record it describes and carried through
snapshot/restore.
already_resampled WAS THE CLI OPTION, NOT WHAT EACH PASS DID. The draw is skipped per pass when
it would not shrink that pass's record, so one global boolean either flattens a replica whose
importance weights are genuine, or leaves a resampled replica double-weighted -- and a run near
the n_extr boundary produces a MIXTURE, which a boolean cannot describe at all.
_pool_replica_rvs now takes a per-replica sequence and decides per block; the ILE captures each
replica's marker beside the record it describes. While there: the empty-record filter is now
lockstep. It dropped from rep_rvs alone, shifting every later block against its own lnZ -- and
would now have shifted it against its own resampled flag too.
Tests: 10 new (38 in the suite), including a raw+resampled mixture that pins BOTH directions --
the raw block keeps its weight shape, the resampled block is flattened, and both still carry
Z_k/K. Verified by reverting each fix: the property split fails 4 tests, the per-replica
provenance fails 1. Three tests from the previous round were stale against the new contract and
were updated rather than deleted.
214 passed, 3 skipped across the integrator suites; --check green at 132 sites.
GitHub Actions
Failure
|
Both fixed in 1. One flag, two questionsConfirmed exactly as described.
Clearing it after pooling fixed the second and broke the first in two places, both verified by Now two predicates over two markers — 2.
|
…turn _reject_if_collapsed RAISES after pooling, the caller's `except Exception` swallows it and moves to the next event, and _rvs_is_pooled survived. The next ordinary fair draw was then read as pooled, _rvs_is_equal_weight went False, and .dgrid and the extrinsic-proposal breadcrumb applied importance weights to rows that already carried them -- the w^2 defect this branch exists to remove, resurrected on the event after any failure. Reset on ENTRY to analyze_event. Entry is reached on every call by construction, needs no restructuring of a 2000-line function, and stays correct for a caller that never returns normally at all; a `finally` would buy nothing extra here and would mean wrapping the whole body. The end-of-function clear stays as well, so a sampler handed to anything else afterwards is not carrying a stale marker. Tests pin three things: the reset exists and sits strictly before anything that can raise or export; the raising gate really does run after pooling (so the test cannot quietly stop testing anything if that order changes); and the numeric consequence of a stale marker -- the helper stops returning uniform weights for a plain fair draw and hands back w. Verified by removing the reset: the ordering test fails. FOURTH round, fourth defect in the provenance bookkeeping rather than in the physics. Recorded as Finding 7 with the four recurring shapes, because the pattern is now the argument for the naming change, not an incidental observation. 217 passed, 3 skipped; --check green at 132 sites.
GitHub Actions
Failure
|
Fixed in Reset on entry to Three tests: the reset exists and sits strictly before anything that can raise or export; the On the pattern. That's four rounds, four defects, all in the provenance bookkeeping and none
Every one was a second source of truth about Opened #94 as a draft for that: I'd rather that be argued separately than ride along here. |
GitHub Actions
Active
NOT FOR MERGE. One worked example of the proposal in DESIGN_rvs_naming.md, wired into a single sampler, so the shape can be argued about against something concrete instead of against prose. Nothing reads it: this branch is a no-op on every output. THE PROBLEM. sampler._rvs means the RETAINED SET while integrate_log accumulates, and an EXPORT RESAMPLE afterwards. The name does not change, the type does not change, so a consumer written against the first meaning keeps working -- silently -- against the second. THE EVIDENCE THAT THIS IS DESIGN AND NOT LUCK. Nine defects of this shape. Five before the audit (CIP export; L0 seed #78; reject gate #79; reserve cap and its logarithm #84), three found by the mechanical sweep in #87, and then FOUR MORE IN REVIEW OF THAT FIX -- every one of the four in the boolean bookkeeping introduced to describe _rvs from outside, none in the physics: 1. a fix correct in isolation, wrong once pooling ran after it 2. one flag answering two questions (rows-resampled vs globally-equal-weight) 3. the CLI option used where "what this pass actually did" was needed 4. a marker cleared only on the normal return, surviving a raised event Each was a second source of truth that some site touching the first failed to maintain. That is what a naming problem looks like once you refuse to rename anything. WHAT THIS DRAFT CONTAINS. RvsRecord carries rows AND provenance together, and replaces the booleans with named questions -- rows_are_resampled() (per BLOCK, survives pooling), is_equal_weight() (whole RECORD, pooling destroys it), blocks_were_flattened() (a fact about the pooling STEP). Those are the three the flags kept conflating; they are three methods with three names because the failure was never that the answer was hard to compute, it was that one name suggested one question while a caller asked another. Provenance is per-BLOCK, so a mixture of raw and resampled replicas is representable at all. The suite is written as one section per review-round failure shape: each test would have caught its round had provenance lived with the rows from the start. If the design is adopted they justify it; if not, they are the specification any replacement has to satisfy. Deliberately NOT a dict subclass: that would let every existing sampler._rvs[...] keep working against an object whose meaning it does not check, which is the original problem with more steps. Consumers reach for .columns, which is visible in a diff and greppable by the audit. BLAST RADIUS, measured by audit_rvs_fairdraw.py: 306 reads, 131 post-rebind, 7 rebind sites. That is why this is option A (two names, incremental) rather than option B (the fair draw returns a new object), which is the correct end state but cannot be done in one change to code that writes science products. Both are written up, with the trade-offs and three open questions, in DESIGN_rvs_naming.md. Verified no-op: 132 passed, 3 skipped across the AV/L0/portfolio suites, and a collapsed AV pass records n_retained=288 against 1 exported row -- the case the whole line of work is about.
The --check gate merged in #87 caught this draft's new sampler-side _rvs read on the very next change to touch one -- which is the behaviour it was built for, on a case nobody wrote it for. Verdict PER_ROW: the record takes the just-rebound columns as a VIEW plus the pre-draw row count, and reads no statistic of them. It records that they ARE the export resample, at the moment that becomes true.
Agreed direction from review. Deliberately one worked example rather than a sweep, so the shape can be judged before the mechanical part. RESERVE BY REFERENCE, not a copy. retained_points()/retained_lnL()/n_retained() point at the bounded, finite-stratified _warm_seed_reserve. From the measurement: holding raw retained rows costs ~0.9 MB per million nmax for AV (nothing) but ~92 MB per million for a PORTFOLIO, i.e. ~384 MB at nmax=4e6 per ILE process -- and it would be mostly ballast, since the portfolio's finite fraction on the collapsed pass this work is about is ~1e-5. The reserve already keeps the affordable thing, with the exact pre-cap weight total so a capped reserve still yields an unbiased lnZ. A pooled record carries NO reserve: it is a mixture of several passes, so there is no single retained set, and pointing at one arbitrary pass's would be worse than None. AV RECORDS BOTH PATHS. fair_draw when the draw fires, retained when it does not. "Absent" and "not resampled" are different statements, and a consumer forced to distinguish them is back to combining conditions by hand -- which is the failure this design exists to remove. POOLING BUILDS A POOLED RECORD carrying _rep_fairdraw PER BLOCK. That is precisely what the two booleans cannot express, and why a raw/resampled mixture needed a special case in _pool_replica_rvs; the record represents it directly. FIRST CONSUMER MIGRATED: ln_weights_for_posterior, chosen because it is the exact site of the one-flag-two-questions defect, so converting it demonstrates the point instead of merely exercising the API. It trusts a record only when `.columns is rvs` -- _rvs is a mutable dict that may have been replaced since the record was built -- and otherwise falls back to the flags. KEEPING TWO DESCRIPTIONS HONEST is the real cost of A, and four review rounds on #87 were all "two descriptions drifted apart", so it is asserted rather than promised: * the record and the flags agree across retained / fair draw / pooled / pooled-mixed / pooled-raw; * on a real collapsed AV pass the record path and the flag path return BIT-IDENTICAL weights, on both branches -- the conversion is a refactor, and stays checkable until the flags go. 26 record tests; 213 passed, 3 skipped across the integrator suites; --check green at 134 sites (it caught the new sampler-side read again, now classified).
…mplers set it
STEP 2 -- the remaining consumers. .dgrid and the extrinsic-proposal breadcrumb already went
through ln_weights_for_posterior, so they moved with it; the .dslice guard and the pooled n_eff
now ask the record directly. Note each asks a DIFFERENT question, which is the point:
.dslice rows_are_resampled() -- survives pooling; reweighting resampled rows
double-counts whether or not they were pooled
pooled n_eff blocks_were_flattened() -- a fact about the pooling STEP, and keying it on
either other question is what made that branch
dead code in review round 2
weights is_equal_weight() -- whole-record
All three go through ONE lookup, _rvs_record_for(sampler, rvs), which declines a record whose
.columns is not the dict being held: _rvs is replaced in place, so "the sampler has a record"
and "the record describes these rows" are different questions. The PRODUCER at the pooling site
asks a third -- _sampler_keeps_records -- and has its own name rather than an exemption, because
it is about to replace sampler._rvs and would otherwise be told "no record" and silently skip
building the pooled one.
STEP 3 -- all seven rebind sites, wired by one patcher against PR #87's own markers so they are
identical rather than seven hand edits. Each site now resets the record, builds a `retained`
record before the draw, and replaces it with a `fair_draw` record after. The reserve rides
along by reference where the sampler keeps one (AV, portfolio); None elsewhere is the honest
answer rather than a gap.
TWO THINGS FOUND DOING IT, both recorded in the design doc:
* n_retained HAD TO BE CAPTURED EAGERLY. RvsRecord.retained(self._rvs) holds a reference to the
live dict, which the draw then rebinds, so len(record) afterwards returns the POST-draw count.
Reading it made a collapsed pass report n_retained == rows -- "nothing was discarded", the
exact opposite of the truth. This project's own bug class, in the code written to prevent it.
Caught because the end-to-end check printed n_ret == rows and that looked wrong.
* mcsampler and mcsamplerEnsemble take a LINEAR integrand; AV and the portfolio take log. The
wrong kind makes the fair draw compute negative weights and raise. Confirmed to fail
IDENTICALLY on the pristine file before concluding anything, so it is a harness contract, not
a defect I introduced.
Tests: 31 in the record suite, including every wired sampler agreeing with its flags on both
draw settings, and a structural check that all seven sites are wired the same way (one patcher
means one mistake would be replicated everywhere -- the case worth testing rather than eyeballing
a diff). 248 passed, 3 skipped overall; --check green at 141 sites.
Still DRAFT. The flags stay until every consumer is migrated; deleting them is step 4.
…r that owns it
Enumerated mechanically before touching anything, since this touches a lot: 7 sampler
self-reads (the producer reading its own attribute -- those stay), 11 in the ILE of which 2
were WRITES, 14 in the tests.
The two ILE writes are the interesting ones. Replica pooling legitimately PRODUCES a record
the sampler cannot build (it is a mixture of several passes), and with no public writer that
code had to assign sampler._rvs_record directly -- reaching into another object's private
attribute, which is the habit this design exists to end. So the mixin grew set_samples(): a
writer needs an API as much as a reader does.
Also folded the record into _snapshot_pass_state/_restore_pass_state, so everything describing
a pass still moves together. A stale record was already declined by _rvs_record_for's identity
check, so this is belt-and-braces -- but "everything moves together" is the invariant, and
carving an exception into it is how review round 1 happened.
THE BOUNDARY IS NOW A TEST, and getting that test right took three attempts:
1. substring search -- counts the COMMENTS explaining the hazard, which in these files are
most of the occurrences. Same false alarm as PR #87.
2. strip comments, count tokens -- MISSES getattr(sampler, '_rvs_record'), where the name is
a string literal, and that is exactly the form a consumer reaching inside would use. This
version PASSED against a deliberately reintroduced violation: worse than no test. Found
only by revert-checking it, which is the habit that keeps paying.
3. AST -- attribute access where the object is not `self`, plus getattr/setattr/hasattr with
the name as a string constant. Verified to fail on BOTH violation forms and pass on
restore.
The LISA driver gets its own case: as a deliberate fork it may legitimately have none of this,
but "none" and "half" are different, and half is how a fork rots.
271 passed, 4 skipped; both gates green; the ensemble script-style test still recovers AC/GMM/AV
to ~1.0. Still DRAFT; flags still in place.
1 participant
Closes the audit the last four PRs left open: "Every other consumer of
_rvsrows or lengthsin the ILE is unaudited. Everything found here was in code this change happened to touch."
sampler._rvsis not the sample set.integrate_logrebinds every key to a subset drawn WITHREPLACEMENT proportional to weight — an EXPORT resample. Five defects had come from consumers
reading it as the sample set, each found only by accident. This sweep is mechanical and
re-runnable, and everything it found is fixed here rather than left as follow-up.
The census
306 reads; 131 see the resample.
audit_rvs_fairdraw.py --checkruns in CI and fails on anypost-rebind read without a recorded verdict, keyed by a hash of the read itself rather than its
line or its function —
analyze_eventalone holds 40 sites, and every known defect was addednext to correct code. Ledger
BROKEN: 8 → 1 (the remainder is #79's documentedcross-source fallback).
Two results worth keeping:
rebind's own RHS or the cupy→numpy conversion loop. The hazard is entirely in the consumers.
create_event_parameter_pipeline_BasicIteration,cepp_basic_htcondorandcreate_event_nr_pipeline_with_cipall pass--fairdraw-extrinsic-outputto the extrinsic stage unconditionally.Fixed
Sequential warm-start seed.
--sampler-sequential-warmstartread_rvsand guarded on a rowCOUNT — PR #78's defect one code path away. Both directions are quiet: on the measured pass the
draw leaves ONE row so the count declines and the feature is silently inert; at ~5 rows it
accepts a rank-2-of-6 seed and the next point warm-starts into a sliver reporting a healthy
n_eff. Now seeds from the retained reserve and judges by affine RANK. The L0 rescue's inlinelookup is replaced by a shared
_warm_seed_reserve_forso the pair cannot drift.Three double-weighting sites. A fair-drawn record is already equal-weight; weighting it by
wagain givesw²._pool_replica_rvshas guarded against this since the replica work; thesehad no equivalent — the
--extrinsic-proposal-outputGMM breadcrumb (worst: itsover-concentrated proposal is handed to the NEXT iteration, so truncation compounds),
.dgrid,and the
.dslicereweight core. The last is a different shape and cannot be corrected once thepre-draw record is gone, so it is routed to the exact all-fresh path, loudly, since that changes
cost.
The predicate is not the CLI flag: samplers skip the draw when it would not shrink the
record, and those rows still carry real weights — flattening them would be the same error
reversed. All seven rebind sites now mark the rebind itself.
Pooled
n_eff. Kish of the pooled record overwrites the reportedneff, but on a fair-drawnexport
_pool_replica_rvshas flattened each block, and Kish of piecewise-constant weights isjust the row count (5K at the default
--fairdraw-extrinsic-output-n-max 5). Now Kish over theBLOCKS,
(Σ Z_k)² / Σ(Z_k²/neff_k)— reduces toΣ neff_kwhen replicas agree, falls below itwhen they do not.
Measured:
--sampler-l0-rescue-reject-dlnZ, default raised 0.5 → 3.0160 known-lnZ passes across AV and portfolio. The gate caught 0 of 55 genuinely truncated warm
passes at every threshold from 0.25 to 4.0 nats, while at 0.5 it binned 25% of good portfolio
warm passes. 0.5 was strictly dominated.
Rates are conditioned on what the warm pass actually did, not the intended condition — a
portfolio's defensive GMM component means a seeded warm pass usually still reaches every mode
(77 of 80), so scoring against the label would have counted false positives as successes.
On AV the gate's premise is false outright: the collapsed cold pass reports lnZ 70–92 nats
below truth while the warm pass is within a nat. Not a helper bug —
lnZ_from_reservereproduces
integrate_log's own lnZ to the digit. Pre-#79 the+log(n/eff_samp)inflationoccasionally pushed the difference positive, which is where the observed 10-of-12 rejections came
from: the artifact, not the mechanism.
Support containment is the recommended replacement and is deliberately not attempted here — a
new detector wants its own validation.
Tests
34 new across
test_seq_warmstart_seed.pyandtest_fairdraw_double_weighting.py, both added toCI, plus the
--checkgate. 197 passed, 3 skipped across the integrator suites. Measurementscripts, trimmed run logs and a per-replicate JSON dumper are committed, so every number above is
reproducible without re-running the passes.
Reviewing this
Squashed to one commit deliberately: an earlier history had a commit concluding "leave the
default", which the measurement later reversed. The reasoning trail lives in
RVS_FAIRDRAW_AUDIT.mdandL0_REJECT_DLNZ_MEASUREMENT.md, where it stays accurate.Scope limits are stated in the measurement doc: synthetic targets, rho=100, one bimodal geometry,
no real GW likelihoods or GPU backends. The portfolio truncated-pass count is only 3, so the
strong statement comes from AV's 52.