Skip to content

Commit 4c40e2a

Browse files
hmgaudeckerclaude
andcommitted
Repair state-conditioned shocks per the PR #405 code review
pro-math-code-review (5.5-Pro, 2026-07-17) returned serious_gap on the built primitive. All six findings reproduced against the real source and repaired. The design was sound; the implementation had two independent law mismatches that the entire 37-test suite passed over. F1 (serious) — solve gathered the conditioned sigma, simulate did not. The continuous next-state wrappers copied dict(grid.params) and called draw_shock, so every category simulated at the scalar common-grid sigma. Measured on the pre-fix code: the low regime drew std=0.3009 where its law says 0.05, a variance 36x too large. This moves simulated states, moments and every later decision, not just value levels. Both wrappers now take the conditioning state and override sigma via gather_sigma, reusing sigma_array_by_code so the solve and simulate orderings cannot drift apart. F2 (serious) — both row builders dropped a fixed non-zero mu. Stock pylcm builds Tauchen rows in demeaned coordinates, where the intercept vanishes; the conditioned branch is handed physical nodes (centred on mu/(1-rho)) and a physical from-value, so the conditional mean is mu + rho*y and the intercept must appear. Threaded mu through conditioned_row and tauchen_row. F3 — the conditioned branch is selected before the runtime-parameter mechanism, so a grid parameter left for runtime is never bound: get_gridpoints returns all-NaN and the closure captures it forever. Now require is_fully_specified, finite nodes, finite positive sigmas and exact category keys. F4 — sigma is ordered by the target regime's grid but indexed by the source state's code. Require one shared category-to-code map across every regime carrying the conditioning state. F5 — the tests guarded nothing: every reduction test used mu=0 and every model test solved but never simulated. Adds 13 regressions, each verified to fail on the pre-fix code. F6 — the GH rejection was documented as blanket but implemented for IID only. Now rejects Tauchen GH too, matching the stated v1 scope. 50 tests pass (37 before). Lint clean; ty skipped locally (the worktree has no type-checking env) and left to CI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DW9D1WhLN8JhhiEPCScbaw
1 parent 3dd6ef9 commit 4c40e2a

6 files changed

Lines changed: 396 additions & 37 deletions

File tree

CHANGES.md

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,12 @@ chronological order. We follow [semantic versioning](https://semver.org/).
1313
transition row is evaluated directly at the from-value with that regime's `sigma`
1414
(no precomputed-row interpolation). This expresses regime-switching income risk /
1515
stochastic volatility. Supported for CDF-binned `NormalIIDProcess`
16-
(`gauss_hermite=False`) and `TauchenAR1Process`; Gauss-Hermite IID and Rouwenhorst
17-
are rejected at construction (their fixed-node kernels cannot carry a
18-
state-conditioned `sigma`). Current-regime conditioning only. See
19-
`lcm_examples/stochastic_volatility.py`.
16+
(`gauss_hermite=False`) and `TauchenAR1Process`; Gauss-Hermite node placement and
17+
Rouwenhorst are rejected at construction (their fixed-node kernels cannot carry a
18+
state-conditioned `sigma`). Solving and simulating use the same conditioned law.
19+
Every grid parameter must be fixed at construction, and the conditioning state must
20+
map its categories to the same integer codes in every regime that carries it.
21+
Current-regime conditioning only. See `lcm_examples/stochastic_volatility.py`.
2022

2123
- Adds the DC-EGM solver (Iskhakov, Jørgensen, Rust & Schjerning 2017) as a
2224
per-regime alternative to grid search: `Regime(solver=lcm.DCEGM(...))`.

src/_lcm/processes/state_conditioned.py

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,13 +48,21 @@ def sigma_array_by_code(cond_grid: DiscreteGrid, by: Mapping[str, float]) -> Flo
4848
4949
Stacking by `Mapping` insertion order silently permutes regimes when the code order
5050
differs (audit F5/RT5); indexing the returned array by the conditioning state's code
51-
is therefore correct by construction. Every category must appear in `by`.
51+
is therefore correct by construction. `by` must name exactly the categories of
52+
`cond_grid` — an extra key is a typo or a stale category, never a no-op.
5253
"""
5354
cats, codes = cond_grid.categories, cond_grid.codes
5455
missing = set(cats) - set(by)
5556
if missing:
5657
msg = f"StateConditioned.by is missing categories {sorted(missing)}"
5758
raise ValueError(msg)
59+
extra = set(by) - set(cats)
60+
if extra:
61+
msg = (
62+
f"StateConditioned.by has categories {sorted(extra)} that are not in the "
63+
f"conditioning grid {sorted(cats)}"
64+
)
65+
raise ValueError(msg)
5866
# Runtime indexing (`gather_sigma`) uses the conditioning state's code directly,
5967
# which is safe because `@categorical` assigns contiguous 0..n-1 codes and
6068
# `DiscreteGrid` accepts only such classes — so position == code here.
@@ -68,20 +76,23 @@ def conditioned_row(
6876
nodes: Float1D,
6977
sigma: _Scalar,
7078
from_value: _Scalar,
79+
mu: _Scalar,
7180
rho: _Scalar | None = None,
7281
) -> Float1D:
7382
"""Dispatch to the direct-CDF row builder for the given process `family`.
7483
7584
`nodes` are the FIXED common nodes (from `grid_sigma`); `sigma` is the current
76-
regime's innovation std (already gathered by code). `rho` is required for Tauchen.
85+
regime's innovation std (already gathered by code). `mu` is the process's fixed
86+
location (IID mean / AR(1) intercept) — dropping it misplaces the entire row
87+
(code-review F2). `rho` is required for Tauchen.
7788
"""
7889
if family == "iid_normal":
79-
return iid_normal_row(nodes, mu=0.0, sigma=sigma)
90+
return iid_normal_row(nodes, mu=mu, sigma=sigma)
8091
if family == "tauchen":
8192
if rho is None:
8293
msg = "conditioned_row(family='tauchen') requires rho"
8394
raise ValueError(msg)
84-
return tauchen_row(nodes, rho=rho, sigma=sigma, from_value=from_value)
95+
return tauchen_row(nodes, rho=rho, sigma=sigma, from_value=from_value, mu=mu)
8596
msg = f"unsupported family {family!r} (v1: 'iid_normal' | 'tauchen')"
8697
raise ValueError(msg)
8798

@@ -118,12 +129,19 @@ def tauchen_row(
118129
rho: _Scalar,
119130
sigma: _Scalar,
120131
from_value: _Scalar,
132+
mu: _Scalar = 0.0,
121133
) -> Float1D:
122-
"""Conditional AR(1) row on FIXED ``nodes``, innovation ``~ N(0, sigma**2)``.
134+
"""Conditional AR(1) row for ``y' = mu + rho*y + eps``, ``eps ~ N(0, sigma**2)``.
123135
124136
Evaluated DIRECTLY at ``from_value`` (audit F1) rather than by interpolating node
125137
rows. The denominator is the innovation ``sigma`` (conditional std of ``y' | y``),
126138
matching ``TauchenAR1Process.compute_transition_probs``.
139+
140+
``nodes`` and ``from_value`` are in PHYSICAL units — i.e. the axis
141+
``TauchenAR1Process.compute_gridpoints`` returns, centred on ``mu/(1-rho)`` —
142+
so the conditional mean is ``mu + rho*from_value``. Stock pylcm builds the same
143+
row in demeaned coordinates, where the intercept vanishes; here it does not, and
144+
omitting it misplaces every row unless ``mu == 0`` (code-review F2).
127145
"""
128146
edges = (nodes[:-1] + nodes[1:]) / 2.0
129-
return _row_from_edge_cdf(cdf((edges - rho * from_value) / sigma))
147+
return _row_from_edge_cdf(cdf((edges - mu - rho * from_value) / sigma))

src/_lcm/regime_building/next_state.py

Lines changed: 77 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,23 @@
11
"""Generate function that compute the next states for solution and simulation."""
22

3-
from collections.abc import Callable
3+
from collections.abc import Callable, Mapping
44
from types import MappingProxyType
5+
from typing import Any
56

67
import jax
78
from dags import concatenate_functions, with_signature
89
from dags.tree import qname_from_tree_path
910

1011
from _lcm.engine import Variables
11-
from _lcm.grids import Grid
12+
from _lcm.grids import DiscreteGrid, Grid
1213
from _lcm.processes import _ContinuousStochasticProcess
1314
from _lcm.processes.ar1 import _AR1Process
1415
from _lcm.processes.iid import _IIDProcess
16+
from _lcm.processes.state_conditioned import (
17+
StateConditioned,
18+
gather_sigma,
19+
sigma_array_by_code,
20+
)
1521
from _lcm.typing import (
1622
EconFunctionsMapping,
1723
NextStateSimulationFunction,
@@ -24,7 +30,8 @@
2430
TransitionFunctionName,
2531
TransitionFunctionsMapping,
2632
)
27-
from lcm.typing import ContinuousState, DiscreteState, FloatND, IntND
33+
from lcm.exceptions import ModelInitializationError
34+
from lcm.typing import ContinuousState, DiscreteState, Float1D, FloatND, IntND
2835

2936

3037
def get_next_state_function_for_solution(
@@ -273,17 +280,55 @@ def _create_continuous_stochastic_next_func(
273280
grid: _ContinuousStochasticProcess = all_grids[target_regime_name][state_name] # ty: ignore [invalid-assignment]
274281
qname = qname_from_tree_path((target_regime_name, next_state_name))
275282

283+
# A state-conditioned process must DRAW with the current regime's sigma, not the
284+
# scalar common-grid sigma — otherwise solve and simulate run different laws
285+
# (code-review F1).
286+
conditioned = _resolve_conditioned_sigma(
287+
grid=grid, grids=all_grids[target_regime_name]
288+
)
289+
276290
if isinstance(grid, _AR1Process):
277-
return _create_ar1_next_func(qname=qname, state_name=state_name, grid=grid)
291+
return _create_ar1_next_func(
292+
qname=qname, state_name=state_name, grid=grid, conditioned=conditioned
293+
)
278294
if isinstance(grid, _IIDProcess):
279-
return _create_iid_next_func(qname=qname, state_name=state_name, grid=grid)
295+
return _create_iid_next_func(
296+
qname=qname, state_name=state_name, grid=grid, conditioned=conditioned
297+
)
280298

281299
msg = f"Expected _IIDProcess or _AR1Process, got {type(grid)}"
282300
raise TypeError(msg)
283301

284302

303+
def _resolve_conditioned_sigma(
304+
*,
305+
grid: _ContinuousStochasticProcess,
306+
grids: Mapping[StateOrActionName, Grid],
307+
) -> tuple[StateConditioned, Float1D] | None:
308+
"""Resolve the code-ordered sigma array of a state-conditioned process, else None.
309+
310+
Reuses `sigma_array_by_code` so simulation gathers sigma under exactly the
311+
category-code ordering the solve-side transition rows use (code-review F1).
312+
"""
313+
sc = grid.state_conditioned
314+
if sc is None:
315+
return None
316+
conditioning_grid = grids.get(sc.on)
317+
if not isinstance(conditioning_grid, DiscreteGrid):
318+
msg = (
319+
f"state_conditioned.on='{sc.on}' must name a DiscreteGrid state in the "
320+
f"same regime as the process."
321+
)
322+
raise ModelInitializationError(msg)
323+
return sc, sigma_array_by_code(conditioning_grid, sc.by)
324+
325+
285326
def _create_ar1_next_func(
286-
*, qname: str, state_name: StateName, grid: _AR1Process
327+
*,
328+
qname: str,
329+
state_name: StateName,
330+
grid: _AR1Process,
331+
conditioned: tuple[StateConditioned, Float1D] | None = None,
287332
) -> StochasticNextFunction:
288333
fixed_params = dict(grid.params)
289334
runtime_param_names = {
@@ -294,6 +339,8 @@ def _create_ar1_next_func(
294339
state_name: "ContinuousState",
295340
**dict.fromkeys(runtime_param_names, "FloatND"),
296341
}
342+
if conditioned is not None:
343+
args[conditioned[0].on] = "DiscreteState"
297344
_draw_shock = grid.draw_shock
298345

299346
@with_signature(args=args, return_annotation="ContinuousState")
@@ -302,6 +349,7 @@ def next_stochastic_state(**kwargs: FloatND) -> ContinuousState:
302349
{
303350
**fixed_params,
304351
**{raw: kwargs[qn] for qn, raw in runtime_param_names.items()},
352+
**_conditioned_sigma(conditioned, kwargs),
305353
}
306354
)
307355
return _draw_shock(
@@ -314,7 +362,11 @@ def next_stochastic_state(**kwargs: FloatND) -> ContinuousState:
314362

315363

316364
def _create_iid_next_func(
317-
*, qname: str, state_name: StateName, grid: _IIDProcess
365+
*,
366+
qname: str,
367+
state_name: StateName,
368+
grid: _IIDProcess,
369+
conditioned: tuple[StateConditioned, Float1D] | None = None,
318370
) -> StochasticNextFunction:
319371
fixed_params = dict(grid.params)
320372
runtime_param_names = {
@@ -324,6 +376,8 @@ def _create_iid_next_func(
324376
f"key_{qname}": "PRNGKeyND",
325377
**dict.fromkeys(runtime_param_names, "FloatND"),
326378
}
379+
if conditioned is not None:
380+
args[conditioned[0].on] = "DiscreteState"
327381
_draw_shock = grid.draw_shock
328382

329383
@with_signature(args=args, return_annotation="ContinuousState")
@@ -332,6 +386,7 @@ def next_stochastic_state(**kwargs: FloatND) -> ContinuousState:
332386
{
333387
**fixed_params,
334388
**{raw: kwargs[qn] for qn, raw in runtime_param_names.items()},
389+
**_conditioned_sigma(conditioned, kwargs),
335390
}
336391
)
337392
return _draw_shock(
@@ -340,3 +395,18 @@ def next_stochastic_state(**kwargs: FloatND) -> ContinuousState:
340395
)
341396

342397
return next_stochastic_state
398+
399+
400+
def _conditioned_sigma(
401+
conditioned: tuple[StateConditioned, Float1D] | None,
402+
kwargs: Mapping[str, Any],
403+
) -> Mapping[str, Any]:
404+
"""`{"sigma": <current regime's sigma>}` for a conditioned process, else `{}`.
405+
406+
Overrides the scalar common-grid sigma the draw would otherwise use, which is what
407+
makes the simulated law match the solved one (code-review F1).
408+
"""
409+
if conditioned is None:
410+
return {}
411+
sc, sigma_by_code = conditioned
412+
return {"sigma": gather_sigma(sigma_by_code, kwargs[sc.on])}

0 commit comments

Comments
 (0)