Skip to content

Commit ee13ced

Browse files
hmgaudeckerclaude
andcommitted
Repair state-conditioned shocks per PR #405 rounds 2-3 review
Round-2 findings: - F1: broadcast pruner and usage validator missed the metadata-declared `state_conditioned.on` dependency, rejecting legitimate models. Both passes now register the conditioner explicitly (pruner keeps it only when a retained or reachable-target process reads it; validator credits it model-wide). - F2: `_validate_conditioned_grid_is_fixed` now rejects singleton and non-strictly-increasing node axes. - F4: added AR(1) simulate-side regression (innovation std tracks conditioned sigma) and a through-time timing test. Round-3 finding: - F1 (cross-regime): a conditioned process in a target regime creates a dependency in every source regime that can reach it (per-target transitions), evaluated at the source's own `on` state. Pruner keeps the conditioner for reachable targets; validator credits it via the model-wide set. Added three cross-regime regression tests (build + solve + semantic). F3 accepted as a documented v1 limitation (cross-regime code agreement). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DW9D1WhLN8JhhiEPCScbaw
1 parent ca3a170 commit ee13ced

4 files changed

Lines changed: 681 additions & 0 deletions

File tree

src/_lcm/model_processing.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
materialize_granular_transition_params,
2727
)
2828
from _lcm.params.sequence_leaf import SequenceLeaf
29+
from _lcm.processes import _ContinuousStochasticProcess
2930
from _lcm.regime_building.finalize import FinalizedUserRegime
3031
from _lcm.regime_building.h_dag import get_dag_targets_consumed_by_H
3132
from _lcm.regime_building.max_Q_over_a import TASTE_SHOCK_SCALE_PARAM
@@ -257,6 +258,30 @@ def _fail_if_invalid_n_subjects(*, n_subjects: int | None) -> None:
257258
raise ValueError(msg)
258259

259260

261+
def _model_wide_conditioning_names(
262+
user_regimes: Mapping[RegimeName, UserRegime],
263+
) -> set[str]:
264+
"""Every conditioning state read by any state-conditioned process in the model.
265+
266+
`state_conditioned.on` is a real dependency of the generated weights/draw functions,
267+
but it lives in grid metadata rather than in a user function, so a callable-DAG scan
268+
misses it. A conditioned process's transition weight is also built into the Q of
269+
every *source* regime that can reach the process's regime, evaluated at that
270+
source's current `on` state — so the dependency is not local to the process's regime
271+
(round-3 review F1). We collect the conditioners model-wide and, at the call site,
272+
credit each regime for those it actually carries: a conservative over-approximation
273+
of reachability whose only cost is not flagging a genuinely unused state, never a
274+
wrong policy.
275+
"""
276+
return {
277+
grid.state_conditioned.on
278+
for user_regime in user_regimes.values()
279+
for grid in user_regime.states.values()
280+
if isinstance(grid, _ContinuousStochasticProcess)
281+
and grid.state_conditioned is not None
282+
}
283+
284+
260285
def _validate_all_variables_used(
261286
user_regimes: Mapping[RegimeName, UserRegime],
262287
*,
@@ -285,6 +310,7 @@ def _validate_all_variables_used(
285310
286311
"""
287312
error_messages = []
313+
conditioning_names = _model_wide_conditioning_names(user_regimes)
288314

289315
for regime_name, user_regime in user_regimes.items():
290316
variable_names = set(user_regime.states) | set(user_regime.actions)
@@ -306,6 +332,14 @@ def _validate_all_variables_used(
306332
reachable = get_ancestors(
307333
user_functions, targets=targets, include_targets=False
308334
)
335+
# A state-conditioned process reads `state_conditioned.on`: the generated solve
336+
# weights and the simulation draw both take it as an argument. But it is
337+
# declared as grid *metadata* rather than in a user function, so the ancestry
338+
# above cannot see it. A process conditioned in one regime is also drawn into a
339+
# *source* regime's Q when that source can reach it, evaluated at the source's
340+
# own `on` state — so a conditioner is credited to every regime that carries it,
341+
# not only the process's own regime (round-2 review F1 / round-3 review F1).
342+
reachable = set(reachable) | (conditioning_names & variable_names)
309343
unused_variables = sorted(variable_names - reachable)
310344

311345
if unused_variables:

src/_lcm/regime_building/broadcast.py

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
from dags import get_ancestors
2222

2323
from _lcm.grids import Grid
24+
from _lcm.processes import _ContinuousStochasticProcess
2425
from _lcm.regime_building.phases import (
2526
PhasedRegimeSpec,
2627
RegimePhaseSpec,
@@ -264,6 +265,13 @@ def _phase_fixed_point(
264265
reachable_targets=reachable[regime_name],
265266
kept=grown,
266267
)
268+
needed |= _state_conditioned_names(
269+
user_regime=user_regime,
270+
reachable_targets=reachable[regime_name],
271+
user_regimes=user_regimes,
272+
candidates=broadcast_variables[regime_name],
273+
grown_here=grown[regime_name],
274+
)
267275
newly_kept = candidates & needed
268276
if newly_kept:
269277
grown[regime_name] = grown[regime_name] | newly_kept
@@ -272,6 +280,57 @@ def _phase_fixed_point(
272280
return grown
273281

274282

283+
def _state_conditioned_names(
284+
*,
285+
user_regime: UserRegime,
286+
reachable_targets: frozenset[RegimeName],
287+
user_regimes: Mapping[RegimeName, UserRegime],
288+
candidates: frozenset[StateOrActionName],
289+
grown_here: frozenset[StateOrActionName],
290+
) -> set[str]:
291+
"""Collect the conditioning states this regime's Q reads through a process.
292+
293+
A state-conditioned process reads ``state_conditioned.on``: the generated solve
294+
weights and simulation draw both take it as an argument. But it is declared as
295+
*metadata* on the grid rather than in a user function, so `_needed_names`'s
296+
callable-DAG ancestry cannot see it, and a conditioning state declared at model
297+
level (a broadcast candidate, reaching nothing else) was pruned here before the
298+
feature could ask for it — leaving a misleading "must name a DiscreteGrid state in
299+
the same regime" error at build time (code-review round 2, F1).
300+
301+
Two sources contribute:
302+
303+
- **local processes** — declared in this regime. Only a *retained* process forces
304+
its conditioner (a process that is itself a pruned broadcast candidate reads
305+
nothing), so this stays monotone in ``grown_here`` and the caller's fixed point
306+
terminates.
307+
- **reachable-target processes** — a conditioned process in a regime this one can
308+
transition into has its transition weight built into *this* regime's Q, evaluated
309+
at *this* regime's ``on`` state. So the conditioner is needed in the source too,
310+
not only the process's own regime (round-3 review F1).
311+
312+
Keeping the state also keeps its law of motion, which the caller filters by the
313+
same pruned-set.
314+
"""
315+
names: set[str] = set()
316+
for name, grid in user_regime.states.items():
317+
if not isinstance(grid, _ContinuousStochasticProcess):
318+
continue
319+
if grid.state_conditioned is None:
320+
continue
321+
if name in candidates and name not in grown_here:
322+
continue # the process itself is not (yet) retained
323+
names.add(grid.state_conditioned.on)
324+
for target in reachable_targets:
325+
for grid in user_regimes[target].states.values():
326+
if (
327+
isinstance(grid, _ContinuousStochasticProcess)
328+
and grid.state_conditioned is not None
329+
):
330+
names.add(grid.state_conditioned.on)
331+
return names
332+
333+
275334
def _needed_names(
276335
*,
277336
phase_slice: RegimePhaseSpec,

src/_lcm/regime_building/processing.py

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1680,6 +1680,13 @@ def _validate_conditioning_codes_agree_across_regimes(
16801680
do; a regime that relabels `{low: 0, high: 1}` to `{high: 0, low: 1}` would silently
16811681
swap the two volatilities (code-review F4). Rather than thread the source grid
16821682
through every seam, v1 requires one shared map and rejects otherwise.
1683+
1684+
Round-2 review F3 noted this global scan is not edge-precise (it fails a
1685+
state-less reachable source late rather than at the guard, and over-rejects
1686+
disconnected components reusing the name), but established **no silent policy
1687+
error**. Kept deliberately for v1: the conservative rule is safe, and the
1688+
per-edge alternative would need source-reachability plumbing that could itself
1689+
introduce the sigma-swap this prevents. Accepted limitation, not an oversight.
16831690
"""
16841691
maps = {
16851692
regime: dict(zip(grid.categories, grid.codes, strict=True))
@@ -1712,6 +1719,10 @@ def _validate_conditioned_sigmas(by: Mapping[str, float]) -> None:
17121719
raise ModelInitializationError(msg)
17131720

17141721

1722+
#: A CDF row bins on node midpoints, so it needs at least one interior edge.
1723+
_MIN_CONDITIONED_NODES = 2
1724+
1725+
17151726
def _validate_conditioned_grid_is_fixed(
17161727
*, name: str, grid: _ContinuousStochasticProcess
17171728
) -> None:
@@ -1735,6 +1746,30 @@ def _validate_conditioned_grid_is_fixed(
17351746
f"state-conditioned process '{name}' resolved to non-finite nodes: {nodes}"
17361747
)
17371748
raise ModelInitializationError(msg)
1749+
# Finiteness is not enough. The direct-CDF row bins on the MIDPOINTS of these nodes,
1750+
# so a strictly increasing axis of >= 2 points is what makes the CDF differences
1751+
# probabilities at all (code-review round 2, F2). All of this is reachable from the
1752+
# public API: `sigma=-0.3` or `n_std=-3.0` yields a DESCENDING axis (pylcm does not
1753+
# require a positive sigma), `sigma=0.0` collapses every node, and a large `mu` in
1754+
# float32 rounds them together. The descending case is the dangerous one — the row
1755+
# still sums to 1.0, so a row-sum check passes while masses go negative and a
1756+
# continuation decision flips sign.
1757+
if nodes.ndim != 1 or nodes.shape[0] < _MIN_CONDITIONED_NODES:
1758+
msg = (
1759+
f"state-conditioned process '{name}' needs a 1-D axis of at least "
1760+
f"{_MIN_CONDITIONED_NODES} nodes; got shape {nodes.shape}. A single node "
1761+
f"leaves the CDF row no bin edges."
1762+
)
1763+
raise ModelInitializationError(msg)
1764+
if not bool(jnp.all(jnp.diff(nodes) > 0)):
1765+
msg = (
1766+
f"state-conditioned process '{name}' resolved to a node axis that is not "
1767+
f"strictly increasing: {nodes}. Midpoint-CDF binning would return negative "
1768+
f"transition masses (which still sum to 1.0, so this fails silently). "
1769+
f"Check for a negative or zero sigma / n_std, or nodes collapsed by "
1770+
f"float32 precision at a large mu."
1771+
)
1772+
raise ModelInitializationError(msg)
17381773

17391774

17401775
def _get_conditioned_weights_func(

0 commit comments

Comments
 (0)