Validation cleanup — delete error_handling.py and validation.py#359
Open
hmgaudecker wants to merge 9 commits into
Open
Validation cleanup — delete error_handling.py and validation.py#359hmgaudecker wants to merge 9 commits into
hmgaudecker wants to merge 9 commits into
Conversation
Move `collect_state_transitions`, `_make_identity_fn`, and `_add_raw_transition` from `regime_building/validation.py` to a new focused module. Update imports in 5 callers. Drop unused imports from `validation.py`. Part of the Phase 1 effort to delete the "validation" and "error_handling" umbrellas; see `Phase 1 — Validation Cleanup.md`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move 8 validators from `regime_building/validation.py` into `user_regime.py` and privatize the two formerly-public ones: - `validate_mapping_contents` -> `_validate_mapping_contents` - `validate_logical_consistency` -> `_validate_logical_consistency` - `_validate_distributed_grids`, `_validate_function_output_grid_indexing`, `_find_function_output_grid_indexing`, `_validate_active`, `_validate_state_transitions`, `_validate_per_target_dict` The validators are sole-called from `UserRegime.__post_init__`; co-locating them with the class eliminates a misleading umbrella module and the cross-module delayed import in `__post_init__`. Delete `regime_building/validation.py`. Part of Phase 1 — see `Phase 1 — Validation Cleanup.md`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move `_get_func_indexing_params`, `_slice_references_params`, `_collect_subscripts`, `_extract_bare_names` from `utils/error_handling.py` into a new focused module. Update imports in `pandas_utils.py` and `tests/test_validate_array_indexing.py`. `error_handling.py::validate_transition_probs` still uses `_get_func_indexing_params` and now imports it from the new module (deferred to M5' for full extraction). Part of Phase 1 — see `Phase 1 — Validation Cleanup.md`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Create `regime_building/runtime_checks.py` and absorb two families from `utils/error_handling.py`: - V family (`validate_V`, `_enrich_with_diagnostics`, `_summarize_diagnostics`, `_format_diagnostic_summary`) - regime-prob family (`validate_regime_transition_probs` -> `_validate_regime_transition_probs`, `_format_sum_violation`, `validate_regime_transitions_all_periods`, `_validate_regime_transition_single`, `_validate_no_reachable_incomplete_targets`) Both families fit the unifying concept "defensive checks on JAX arrays produced during solve/simulate." Privatize `validate_regime_transition_probs` (only tests call it directly). Update imports in 6 callers (3 src, 3 test). `diagnostics.py` keeps its name. Part of Phase 1 — see `Phase 1 — Validation Cleanup.md`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_regime.py Drop the regime-probs overload — that mode is redundant with `validate_regime_transitions_all_periods` which runs unconditionally during `model.solve()` and `model.simulate()` and additionally checks inactive-regime probability and reachability. Keep the state-probs mode. It is the only defence against four silent-correctness bug classes in user-written MarkovTransition functions for states: wrong-shape broadcasting, values outside [0, 1], rows not summing to 1, and subscript-order swaps relative to the function signature. Move slimmed function + helpers (`_extract_markov_transition`, `_build_grids`, `_build_expected_shape`) to `user_regime.py` next to the `Regime` class they operate on. Update `lcm/__init__.py` import. Drop three regime-probs tests from `tests/test_pandas_utils.py`. Update `docs/user_guide/pandas_interop.md` accordingly. Part of Phase 1 — see `Phase 1 — Validation Cleanup.md`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
All contents have been absorbed: V family + regime-prob family into `regime_building/runtime_checks.py` (M4); AST helpers into `utils/ast_inspection.py` (M3); slimmed `validate_transition_probs` into `user_regime.py` (M5'). The "error_handling" umbrella was misleading from the start — three unrelated concerns under one name. Part of Phase 1 — see `Phase 1 — Validation Cleanup.md`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…me.py Three renames driven by Phase 1's src moves: - `tests/test_error_handling_invalid_vf.py` → `tests/test_invalid_vf.py` (the "error_handling" concept is gone from src). - `tests/test_validate_array_indexing.py` → `tests/test_ast_inspection.py` (AST helpers moved to `lcm/utils/ast_inspection.py`). - Extract the four `validate_transition_probs` state-probs tests from `tests/test_pandas_utils.py` into `tests/test_regime.py` (the function now lives in `lcm/user_regime.py`). Duplicate the three-line `_make_partner_probs_array` helper into the new location rather than imposing a cross-file import. Closes the Phase 1 plan in `Phase 1 — Validation Cleanup.md`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Benchmark comparison (main → HEAD)Comparing
|
hmgaudecker
changed the title
…cks.py regime_building/ is the build-time pipeline (UserRegime → engine.Regime). The runtime checks fire on solve / simulate, not at build, so they don't belong here. Split by caller: - validate_V (+ its diagnostic-attachment helpers) is a tight subroutine of the backward-induction loop in solve_brute.py and the V handed to simulate.py. Moves to solution/validate_V.py (solve owns producing V; simulation imports from there). - The regime-transition-probability pre-flight sweep is called from Model.solve()/simulate() before backward induction runs. Moves to a top-level lcm/_transition_checks.py — orthogonal to solution/ and simulation/, sibling of engine.py and model_processing.py. Imports updated in model.py, solve_brute.py, simulate.py and three tests. Docstring on user_regime.validate_transition_probs updated to point at the new location. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- `_transition_checks.py` module docstring no longer references a non-existent `regime_building/static_checks.py`; it describes the actual distinction (runtime numerical checks vs. construction-time regime-spec validators). - `regime_building/transitions.py`: the `collect_state_transitions` docstring said ShockGrid states get a `lambda: None` stub; the code skips them entirely. Docstring corrected. - `solution/validate_V.py`: `Dict` → `dict` in a Returns section to match the `dict[str, Any]` annotation. - `_transition_checks.py` `_validate_no_reachable_incomplete_targets`: drop the override that, when a target regime was absent from the source's `state_transitions`, listed every state of the target as "missing" — including non-stochastic states that need no explicit entry. The preceding line already computes the correct stochastic- only set for that case. - `user_regime.py`: add a scope-boundary note to `_validate_function_output_grid_indexing` — the AST check is deliberately best-effort and should be deleted rather than hardened if it ever produces false positives. - Replace the two contentless happy-path validator tests with boundary inputs: values at the inclusive [0, 1] bounds and row sums just inside the sum-to-1 tolerance, so "does not raise" pins the tolerance/bound logic instead of being a bare smoke check.
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.
Summary
Our validation strategy was not clear. Most of the time we had it coupled in the modules where the stuff was happening, but we also had two umbrella modules whose names misled about what lived inside (
lcm/utils/error_handling.pyandlcm/regime_building/validation.py). This PR absorbs each validator into the module it actually serves. Furthermore, it streamlines validation by removing redundancies and fixing a couple of inconsistencies.File-level outcome
lcm/utils/error_handling.pylcm/regime_building/validation.pylcm/regime_building/transitions.pycollect_state_transitions+ 2 helpers)lcm/_transition_checks.pylcm/solution/validate_V.pylcm/utils/ast_inspection.pylcm/user_regime.pyvalidate_transition_probslcm/pandas_utils.pyutils/ast_inspection)lcm/regime_building/diagnostics.pyNote:
lcm/user_regime.pyblows up to 744 LOC and the publicvalidate_transition_probssits below the eight privateRegime.__post_init__validators — a temporary violation of the "public API first" layout rule. This is purely transient if as PR #360 removesvalidate_transition_probs(-175 LOC). If we decide that PR #361 has the right architecture (it splits this file intoapi/regime.py(class + public function) andlcm/_regime/(validators, transition-probs helpers), that would reduce further.Behaviour changes
validate_transition_probsis now state-probs-only. The old function was overloaded: withstate_nameit validated a state-transition array, without it (state_name=None) it validated a regime-transition array. The regime-probs overload is redundant withvalidate_regime_transitions_all_periods, which runs unconditionally on everymodel.solve()/model.simulate()and additionally checks inactive-regime probability and reachability. The slimmed version requiresstate_name. The overload'sTypeError("does not have a stochastic regime transition")guard is removed along with it — that error surface no longer has a reachable code path; misuse is caught instead byvalidate_regime_transitions_all_periodsat solve/simulate time.validate_regime_transition_probsis now private (_validate_regime_transition_probs). Tests adjust their imports; no other src caller exists._validate_no_reachable_incomplete_targets(relocated into_transition_checks.py) had an override that, when a target regime was entirely absent from the source'sstate_transitions, listed all of the target's states as "missing" — including non-stochastic ones that need no explicit entry. The override is dropped; the error now lists only the stochastic transitions actually required.tests/test_pandas_utils.pywere dropped:_validand_wrong_shapeare subsumed byvalidate_regime_transitions_all_periods;_not_markov_raisesasserted theTypeErrorguard that is removed above, so it goes with the overload. Docs updated accordingly. Everything else is pure mechanical absorption + import-path updates.🤖 Generated with Claude Code