fix(identification): weakref-validated posterior cache for ProxySVAR - #234
Merged
Conversation
Caches keyed on `id(posterior)` return a stale value silently once the cached posterior is collected and its address is recycled by a new object of matching shape. Not reproduced in 200 forced-GC attempts, but the failure mode is wrong numbers with no error. Add `_PosteriorCache`, a single-slot memo that stores `weakref.ref(owner)` alongside the scalar key tail and treats a dead referent — or a live referent that is not the object being looked up — as a miss. The identity check `ref() is owner` subsumes an `id()` comparison exactly, so no `id()` is kept in the key. Owners that cannot be weakly referenced make `set` decline to cache rather than fall back to an unsafe key. Port `ProxySVAR._impact_cache` to it, preserving cache-hit semantics: the weak-instrument warning and the first-stage diagnostics are still produced on the computing call only, so the per-t identification loop pays for the residual reconstruction once. `MaxShare._spectral_cache` and `LongRunRestriction._lr_cache` land on unmerged branches; the helper docstring records the one-line adoption for each. Refs #203 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Egjd7ToFeb9TQqFnfRQZxV
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #234 +/- ##
=====================================
Coverage 93.3% 93.4%
=====================================
Files 41 41
Lines 2506 2539 +33
Branches 298 301 +3
=====================================
+ Hits 2339 2372 +33
Misses 119 119
Partials 48 48 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
thomaspinder
added a commit
that referenced
this pull request
Jul 29, 2026
`LongRunRestriction._lr_cache` keyed its memoised long-run screen on `id(posterior)`. An address is not a validity token: once the posterior is collected the address can be reused, and a lookup with a different posterior that lands on it returns another model's `M` and singular mask silently. Swap it for the weakref-validated `_PosteriorCache` helper added in #234, which ties the entry to `weakref.ref(posterior)` and treats a dead or different referent as a miss. Semantics are unchanged: the screen still computes once per (posterior, n_lags), and `_report` still fires its singular/explosive warnings only on a miss, so the per-t identification loop warns once rather than T times. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Egjd7ToFeb9TQqFnfRQZxV
thomaspinder
added a commit
that referenced
this pull request
Jul 29, 2026
`MaxShare._spectral_cache` keyed its memoised frequency sweep on `id(posterior)`. An address is not a validity token: once the posterior is collected the address can be reused, and a lookup with a different posterior that lands on it returns another model's accumulator `K`, singular mask and spectral radii silently. Swap it for the weakref-validated `_PosteriorCache` helper added in #234, which ties the entry to `weakref.ref(posterior)` and treats a dead or different referent as a miss. Semantics are unchanged: the sweep still computes once per (posterior, n_lags, target_index), and `_report` still fires its singular/explosive warnings only on a miss, so the per-t identification loop warns once rather than T times. `_warn_weakly_identified` is untouched — the eigenvalue ratio depends on `L`, so it is legitimately evaluated on every call. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Egjd7ToFeb9TQqFnfRQZxV
thomaspinder
added a commit
that referenced
this pull request
Jul 29, 2026
`LongRunRestriction._lr_cache` keyed its memoised long-run screen on `id(posterior)`. An address is not a validity token: once the posterior is collected the address can be reused, and a lookup with a different posterior that lands on it returns another model's `M` and singular mask silently. Swap it for the weakref-validated `_PosteriorCache` helper added in #234, which ties the entry to `weakref.ref(posterior)` and treats a dead or different referent as a miss. Semantics are unchanged: the screen still computes once per (posterior, n_lags), and `_report` still fires its singular/explosive warnings only on a miss, so the per-t identification loop warns once rather than T times. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Egjd7ToFeb9TQqFnfRQZxV
thomaspinder
added a commit
that referenced
this pull request
Jul 29, 2026
…ns (#183) * feat(identification): LongRunRestriction — Blanchard-Quah long-run zeros (#143) Adds a closed-form long-run (cumulative) identification scheme. Where Cholesky and sign restrictions constrain the impact matrix Theta(0) = P, this constrains Theta(1) = C(1) P, the cumulative effect of each shock on each variable's level, with C(1) = (I - sum_j A_j)^-1. Construction uses the QR route: with L L' = Sigma, factor (M^-1 L)' = Q R and set P = L Q, sign-fixed so diag(R) > 0. Equivalent to the textbook P = M chol(C(1) Sigma C(1)'), but P P' = Sigma holds exactly (P is L times an orthogonal matrix) and the conditioning of C(1) Sigma C(1)' is never squared. It also makes explicit that the scheme is a rotation of the Cholesky factor, solved in closed form rather than searched for. Scope is recursive (triangular) patterns only; arbitrary non-recursive long-run zeros need the ARW machinery and are deferred to #144. Two failure modes are separated. A near-singular M means C(1) does not exist numerically: those draws are NaN'd (on_undefined="nan", default) or raise, screened by np.linalg.cond against max_condition. Explosive draws (companion spectral radius above 1) are an interpretation failure, not an arithmetic one, so they are always warned about and never blanked — posteriors near a unit root routinely contain them. The screen is memoised on (id(posterior), n_lags) so the per-t SV path does not redo the eigendecomposition, or re-warn, T times. Rows of the returned matrix follow the data's variable order; ordering only fixes the coordinates in which Theta(1) is triangular. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Egjd7ToFeb9TQqFnfRQZxV * fix(identified): keep undefined FEVD draws NaN instead of reporting 0.0 (#143) fevd()'s zero-total guard exists for the degenerate horizon-0 case, but `NaN > 0` is False, so a NaN draw fell through to the 0.0 branch and was reported as a clean "this shock explains nothing" share. Harmless while every scheme returned finite matrices; LongRunRestriction blanks draws whose long-run multiplier is numerically undefined, which exposes it. Extracted the normalisation into _shares() and re-imposed NaN after the guard. Behaviour is unchanged for finite inputs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Egjd7ToFeb9TQqFnfRQZxV * feat(identification): export LongRunRestriction and pin the pipeline behaviour (#143) Adds the lazy export plus end-to-end tests through FittedVAR -> IdentifiedVAR: shock-matrix labelling and diagnostics attrs, FEVD shares, historical-decomposition additivity, and NaN-draw survival through both. The headline test computes the long run twice by independent routes — imposed as Theta(1) = G at identification time, then recovered by summing 200 horizons of the IRF (0.6^200 is around 1e-45, so the truncation is immaterial). Agreement to 1e-8 catches transpose and lag-order mistakes that a self-consistency check would miss. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Egjd7ToFeb9TQqFnfRQZxV * test(identification): from_zero_restrictions and long-run recovery (#143) Covers the named-zeros constructor (happy path, kwarg forwarding, and every rejection branch) and an end-to-end recovery check: simulate 2000 observations from the known truth, fit a ConjugateVAR, identify, and confirm the posterior-median impact matrix lands within 0.05 of the matrix the data were generated from (observed error 0.023). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Egjd7ToFeb9TQqFnfRQZxV * docs(identification): long-run restrictions explanation, how-to, and vocabulary (#143) Adds the "Long-run restrictions" section to the identification explainer (Theta(0) vs Theta(1), the C(1) formula, the closed-form construction, the sign convention, and the two failure modes), a how-to covering both constructors, the diagnostics surface, and the undefined-draw policy, and the Blanchard-Quah bibliography entry. Both pages lead with the differencing requirement. It is the assumption that makes the restriction mean what users think it means, and the library cannot check it. CONTEXT.md gains the long-run multiplier C(1) and the cumulative MA impact matrix Theta(1), plus a relationships bullet recording that schemes may consume posterior lag coefficients — L_t is the minimum a scheme asks for, not the maximum. Also lists ProxySVAR in the identification reference page, which was missing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Egjd7ToFeb9TQqFnfRQZxV * test(identification): cover the multi-lag path and the ordering guard (#143) The p = 1 tests cannot see a lag-block mistake: with one lag the long-run sum has one term and the companion matrix has no subdiagonal. Adds a VAR(2) case that checks identification against the truth, brute-force sums 400 moving-average coefficients to confirm the cumulative impact is the imposed G, and checks the companion spectral radius against a hand- built companion matrix. Also covers n_lags inference from B's trailing axis and the guard that rejects an ordering not covering every variable — silently wrong otherwise. identification.py is now at 99% line coverage, the remainder pre-existing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Egjd7ToFeb9TQqFnfRQZxV * fix(identification): reserved-prefix guard covers the shock_names=None fallback With shock_names=None, shock_coords falls back to the ordering, so a variable literally named unidentified_* leaked the reserved prefix into the shock labels and downstream FEVD/HD masking. Validate the effective labels instead of shock_names alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Egjd7ToFeb9TQqFnfRQZxV * refactor(identification): adopt _PosteriorCache (#203) `LongRunRestriction._lr_cache` keyed its memoised long-run screen on `id(posterior)`. An address is not a validity token: once the posterior is collected the address can be reused, and a lookup with a different posterior that lands on it returns another model's `M` and singular mask silently. Swap it for the weakref-validated `_PosteriorCache` helper added in #234, which ties the entry to `weakref.ref(posterior)` and treats a dead or different referent as a miss. Semantics are unchanged: the screen still computes once per (posterior, n_lags), and `_report` still fires its singular/explosive warnings only on a miss, so the per-t identification loop warns once rather than T times. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Egjd7ToFeb9TQqFnfRQZxV --------- Co-authored-by: Claude Fable 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
ProxySVAR's memoisation was keyed onid(posterior)— if a dead posterior's address were reused by a new object of matching shape, a stale cached impact matrix would return silently (unreproduced in 200 forced-GC attempts, but the exposure is real and the failure mode is wrong numbers with no signal).New module-level
_PosteriorCache(single-slot,__slots__): validity token isref() is owner— a dead referent dereferences toNone, which can never be an owner, so one check subsumes both "collected" and "different object", and noid()survives in the key. Non-weakref-able owners decline to cache (lose the speed-up, never an unsafe key). Verified in-venv that both owners ProxySVAR keys on (xr.Dataset,VARData) support weakrefs and thatidata.posteriorhas stable identity across accesses, so the per-t loop's warn-once and compute-once semantics are preserved exactly (pinned by call-count and warning tests).The helper's docstring records the one-line adoptions for
MaxShare._spectral_cache(#200) andLongRunRestriction._lr_cache(#183) — I'll fold those in during each branch's post-merge rebase, which completes #203.Refs #203 (closes fully once the two branch adoptions land)
Tests
+20: weakref mechanics (dead-referent miss after
gc.collect(), live-different-object miss, tuple/single-owner normalisation, non-weakref-able decline), ProxySVAR cache-hit/invalidation via monkeypatched call counts, and the warn-once pin. Fast suite: 628 passed, 29 deselected; ruff/ty clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01Egjd7ToFeb9TQqFnfRQZxV