fix(scenario): finite shock-matrix guard; infer shock order in from_zero_restrictions - #223
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #223 +/- ##
=====================================
Coverage 95.0% 95.0%
=====================================
Files 45 45
Lines 3099 3109 +10
Branches 380 381 +1
=====================================
+ Hits 2945 2955 +10
Misses 111 111
Partials 43 43 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
thomaspinder
force-pushed
the
feat/143-long-run-restrictions
branch
2 times, most recently
from
July 29, 2026 14:11
fcbb968 to
abff7b2
Compare
thomaspinder
changed the base branch from
feat/143-long-run-restrictions
to
main
July 29, 2026 20:54
…unts Closes #187, closes #188. #187: `counterfactual` and `structural_scenario` are linear solves in the structural shock matrix P. With NaN draws — reachable today via `LongRunRestriction(on_undefined="nan")`, and soon via other schemes — the in-sample engine inverted P and returned an all-NaN counterfactual silently, while the forecast engine died inside LAPACK as "SVD did not converge" from `matrix_rank`. Neither named the cause. A single guard, `_require_finite_shock_matrix`, now runs where P enters each engine (`structural_shock_context` and `_forecast_shock_matrices`), counts the NaN draws, names the `on_undefined="nan"` policy as the likely cause, and points at `on_undefined="raise"` to catch it at identification time. #188: `from_zero_restrictions` rejected genuinely recursive patterns whose `shock_names` were not pre-sorted, though the order is recoverable. The variable ordering already came from the restriction counts; the same count read down the other axis gives the shock order — in a triangular pattern the variable at position i is restricted by the shocks at positions i+1..n-1, so the shock at position k appears in exactly k restriction lists. Ties (non-recursive patterns) sort stably and are still caught by the unchanged exact per-position set check, so the "not recursive" error paths keep their messages. The caller's `shock_names` order is now irrelevant to validity; the names still label the columns. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Egjd7ToFeb9TQqFnfRQZxV
thomaspinder
force-pushed
the
fix/187-188-longrun-followups
branch
from
July 29, 2026 21:39
cb2facb to
d41b593
Compare
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
Two follow-ups on the long-run-restrictions branch (#183), stacked on
feat/143-long-run-restrictions:LongRunRestriction(on_undefined="nan"), and soon MaxShare/ZeroSignRestriction) hit the scenario engines as either an opaqueSVD did not converge(structural_scenario) or — worse than the issue knew — a silent all-NaN counterfactual (np.linalg.invon NaN does not raise on NumPy 2.4). A single_require_finite_shock_matrixguard now sits at the two seams where P enters the engines, counting the offending draws and pointing aton_undefined="raise". IRF/FEVD/HD deliberately keep propagating NaN (pinned by existing tests) — the guard is scenario-only. Tests assert the new error is not aLinAlgError(which subclassesValueError).from_zero_restrictionsrejected genuinely-recursive patterns whoseshock_namesweren't pre-sorted. The shock order is now inferred — a shock's position equals the number of restriction lists it appears in (derived from the triangularity structure) — with the exact per-position set check unchanged as final validation, so non-recursive patterns still fail with the same messages. One existing test had encoded the bug (a valid-but-unsorted pattern asserted as rejected) and was rewritten to a genuine violation.Closes #187
Closes #188
Gates
156 targeted tests green; branch fast suite 580 passed; ruff/ty clean.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Egjd7ToFeb9TQqFnfRQZxV