_rvs audit: derive importance weights instead of reading the ambiguous log_weights cache - #55
Merged
Merged
Conversation
…eights cache Follow-up sweep from PR #51. Two real defects, both of the one-of-two-paths kind. [1] 'log_weights' does not mean one thing. mcsamplerPortfolio stores the true importance weight (lnL + ln p - ln p_s); mcsamplerGPU stores tempering_exp*lnL + ln p - ln p_s -- the ADAPTATION weight. That exponent is not 1 in production: helper_LDG_Events sets it from the SNR (helper_LDG_Events.py:1472/1477), and --no-adapt drives it to 0, removing the likelihood from the column entirely. The .dgrid and calibration-posterior exporters both PREFERRED that cache, so with the GPU/AC sampler they reweighted their output by L^(e-1) -- and by 1/L under --no-adapt. The extrinsic-proposal fit in the same file already derived for exactly this reason, with a comment naming the hazard; these two sites never got the same treatment. Adds ln_weights_from_rvs() as the single definition -- log components, then the linear mcsamplerEnsemble form with out-of-support rows at -inf, and an explicit exception when neither is present, because a loud failure beats a plausible wrong number in a science output. Five call sites now share it, including the two inline copies added in #51. The cache is not read on any weight path. [2] mcsamplerGPU.py:1194 appended new weights onto joint_s_prior instead of weights, corrupting that column from the second chunk on. mcsampler.py:571 carries the identical block WITH the fix and a BUGFIX comment; the GPU copy never received it. Reachable -- mcsamplerGPU:1536 reads _rvs['weights']. Fixed, with a comment naming its twin. Adds test_rvs_weight_derivation.py (5 checks): derives from components rather than the cache and asserts the cache was materially different; --no-adapt loses the likelihood entirely; a portfolio-style cache agrees but is still not read; the linear form sends out-of-support rows to -inf, not NaN; a cache-only record raises instead of guessing. Full verdict table for every writer and reader in RVS_CACHE_AUDIT.md, including one divergence NOT chased: mcsampler.py:1138 notes _rvs['weights'] is sorted as a side effect, and two callers already work around it by recomputing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
oshaughnessy-junior
added a commit
that referenced
this pull request
Aug 11, 2026
…ropped Review of #70 (P3): the previous commit narrowed "AV passes at every budget" to "all 8 seeds at each of the three budgets measured" in FOLLOWUPS item 7, but left the duplicate of that same claim in the CELL_BUDGET_MULT comment reading "at every budget". Now "at each measured budget". Which is, precisely, the defect this PR's item 2 fix and the whole #47/#51/#55 series keep finding: one fact with two representations, the secondary copy goes stale, both plausible read in isolation. Committing it while editing the file whose purpose is to warn about it is the joke telling itself. Swept the rest of the suite for the same shape rather than fixing only the reported line. One other latent instance: item 5's "PASS is 1/8 at every budget from x2 up" could be read as covering untested budgets above x32; now "every measured budget from x2 up". The remaining hits are correctly scoped already -- "no budget tested up to x32 ... at every seed" and "passes 8/8 at every budget measured". Reflowed the comment to the file's ~100 col convention. No numbers, decision or behaviour changed. 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
1 participant
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.
Follow-up sweep from #51: I asked the same question -- "does this consumer prefer a cached column over the canonical components?" -- of every
_rvswriter and reader in the tree. It found a live science-output defect, not just tidiness.log_weightsdoes not mean one thing_rvs['log_weights']mcsamplerPortfolio(:1531/1536)lnL + ln p - ln p_s-- the TRUE importance weightmcsamplerGPU(:766/771)tempering_exp*lnL + ln p - ln p_s-- the ADAPTATION weighttempering_expis the adapt-weight-exponent, and it is not 1 in production:helper_LDG_Eventssets it from the SNR (helper_LDG_Events.py:1472/1477), and--no-adaptdrives it to 0, which removes the likelihood from the column altogether.The
.dgridand calibration-posterior exporters both preferred that cache. So with the GPU/AC sampler they were reweighting their output byL^(e-1)-- and by1/Lunder--no-adapt.The tell was already in the file: the extrinsic-proposal fit ~30 lines earlier derives from the components deliberately, with a comment naming this exact hazard. Two neighbouring consumers never got the same treatment.
Second defect, same shape
mcsamplerGPU.py:1194appended the new weights ontojoint_s_priorinstead ofweights, corrupting that column from the second chunk onward:mcsampler.py:571carries the identical block with the fix and an explicitBUGFIXcomment; the GPU copy never received it. Reachable --mcsamplerGPU:1536reads_rvs['weights']. Fixed, with a comment naming its twin so the pair stays visible.Structural fix
ln_weights_from_rvs()is now the single definition of "the importance weight of an_rvsrecord": log components first, then the linear (mcsamplerEnsemble) form with out-of-support rows at-inf, and an explicit exception when neither is present -- a loud failure beats a plausible wrong number in a science product. Five call sites share it, including the two inline copies I added in #51. The ambiguous cache is no longer read on any weight path.Tests
test_rvs_weight_derivation.py, 5 checks: derives from components and asserts the cache was materially different (so it fails on the old behaviour);--no-adaptloses the likelihood entirely; a portfolio-style cache agrees but is still not read; the linear form sends out-of-support rows to-infrather than NaN; a cache-only record raises instead of guessing.Full audit table
RVS_CACHE_AUDIT.mdrecords a verdict for every writer and reader. It includes one divergence I did not chase:mcsampler.py:1138notes_rvs['weights']is sorted as a side effect, and two callers already work around it by recomputing. That is a third instance of the same theme, pre-existing, and deserves its own pass rather than being folded in here.Gate
COMPARE_EXIT=0, 0 blocking regressions. The probe's single opt-in row is the known pre-existingd4_n1_s303; #54 (probe confirm-on-fail) is the fix for it and is awaiting review.Suites: 22 portfolio, 9 pooling, 5 weight-derivation (new), 7 confirm-on-fail, 5 escaped-mass, 6 sequential-GMM.