Repository navigation
refactor: extract MA recursion + lift time-dim guard to base (closes #76) - #77
Merged
Merged
Conversation
10 tasks done
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #77 +/- ##
=======================================
- Coverage 93.3% 92.2% -1.2%
=======================================
Files 26 27 +1
Lines 1148 1172 +24
Branches 111 114 +3
=======================================
+ Hits 1072 1081 +9
- Misses 55 68 +13
- Partials 21 23 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
thomaspinder
force-pushed
the
audit/02-time-varying-flag
branch
from
May 18, 2026 21:44
48b5156 to
83543bb
Compare
) The MA structural recursion `Phi_h = sum_{j=1}^{min(h,p)} A_j @ Phi_{h-j}` was implemented twice — vectorised over `(C, D)` via einsum in `identified._ma_coefficients`, and per-single-rotation inside `identification.SignRestriction._check_restrictions_at_horizons`. Both copies risked drifting whenever one was fixed. Similarly, `_guard_no_time_dim` was copy-pasted between `IRFResult` and `FEVDResult` with the only difference being the literal `"irf"` / `"fevd"` key. - src/impulso/_ma.py (new): single helper `compute_ma_phi(A: list[np.ndarray], horizon: int) -> np.ndarray`. Numpy `@` broadcasts over leading dims, so the helper handles both single-draw `(n, n)` and batched `(C, D, n, n)` inputs without branching. `np.stack(..., axis=-3)` lands the horizon axis third-to- last regardless of leading rank. - src/impulso/identified.py: `_ma_coefficients` is now a 2-line adapter splitting `B_draws` into the `A` list and delegating. - src/impulso/identification.py: `SignRestriction._check_restrictions_at_horizons` delegates to `compute_ma_phi` and loops horizons applying restrictions on `Phi[h] @ candidate`. - src/impulso/results.py: `_guard_no_time_dim` lifts onto `VARResultBase`. Each subclass declares `_PRIMARY_KEY: ClassVar[str]` (`"irf"` on `IRFResult`, `"fevd"` on `FEVDResult`). The two duplicated copies are deleted. Error message preserves the "time-varying IRFs/FEVDs (at='all')" substring that existing tests match against. - tests/test_ma.py (new): 10 tests covering single-draw, batched, `horizon=0`, `horizon < n_lags`, byte-for-byte agreement with the pre-refactor recursion on a fixed seed, plus input validation. Closes audit findings #2 and #3 (reports/audit-2026-05-18.md). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thomaspinder
force-pushed
the
audit/03-extract-helpers
branch
from
May 18, 2026 21:55
04cbae5 to
f450066
Compare
thomaspinder
added a commit
that referenced
this pull request
May 18, 2026
Wraps up the spring-cleaning stack. Individually small fixes that cluster naturally into one style PR. - src/impulso/volatility.py: `Constant.cholesky_path` / `forecast_cholesky_path` docstrings reference `L` (the actual posterior key after #69) instead of `Sigma`; cross-ref `Constant.cholesky_at` as the canonical accessor. - src/impulso/sv/data.py: `_MIN_OBS` is now `ClassVar[int] = 24` — makes the class-attribute intent explicit instead of relying on Pydantic's implicit underscore handling. - src/impulso/fitted.py: `FittedVAR.has_exog` drops `@computed_field` (now a plain `@property`) so it stops appearing in `model_dump()`. - src/impulso/sv/dynamics.py: `RandomWalk.build_latent_path` / `AR1.build_latent_path` swap `Any` for `"pt.TensorVariable"` forward refs that match the SVDynamics Protocol declaration; drop the now-unused `Any` import. - src/impulso/results.py: delete `VARResultBase.__repr__` override — Pydantic's default produces a useful compact repr because every `idata` field has `Field(repr=False)`. - src/impulso/**/*.py: docstring sweep — double backticks (RST) → single backticks (Markdown / mkdocstrings); no more `:meth:` / `:class:` cross-refs (replaced with plain backticked names in earlier passes). - CLAUDE.md: codify the docstring convention under "Code Conventions". - src/impulso/identified.py: delete `forecast_error_variance_decomposition`; `fevd` is the canonical name. Docstring + .qmd / .md cross-references updated. - pyproject.toml + uv.lock: bump `0.0.5 → 0.0.6` to ship the cumulative audit cleanup (#73 + #75 + #77 + this PR). Closes audit findings #5, #8, #12, #13, #14, #15, #16 (reports/audit-2026-05-18.md). Breaking (pre-v0.1): callers of `IdentifiedVAR.forecast_error_variance_decomposition` must use `IdentifiedVAR.fevd` (no signature change). The long name was always an alias; per repo policy pre-v0.1 API breaks are in scope. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thomaspinder
added a commit
that referenced
this pull request
May 18, 2026
Wraps up the spring-cleaning stack. Individually small fixes that cluster naturally into one style PR. - src/impulso/volatility.py: `Constant.cholesky_path` / `forecast_cholesky_path` docstrings reference `L` (the actual posterior key after #69) instead of `Sigma`; cross-ref `Constant.cholesky_at` as the canonical accessor. - src/impulso/sv/data.py: `_MIN_OBS` is now `ClassVar[int] = 24` — makes the class-attribute intent explicit instead of relying on Pydantic's implicit underscore handling. - src/impulso/fitted.py: `FittedVAR.has_exog` drops `@computed_field` (now a plain `@property`) so it stops appearing in `model_dump()`. - src/impulso/sv/dynamics.py: `RandomWalk.build_latent_path` / `AR1.build_latent_path` swap `Any` for `"pt.TensorVariable"` forward refs that match the SVDynamics Protocol declaration; drop the now-unused `Any` import. - src/impulso/results.py: delete `VARResultBase.__repr__` override — Pydantic's default produces a useful compact repr because every `idata` field has `Field(repr=False)`. - src/impulso/**/*.py: docstring sweep — double backticks (RST) → single backticks (Markdown / mkdocstrings); no more `:meth:` / `:class:` cross-refs (replaced with plain backticked names in earlier passes). - CLAUDE.md: codify the docstring convention under "Code Conventions". - src/impulso/identified.py: delete `forecast_error_variance_decomposition`; `fevd` is the canonical name. Docstring + .qmd / .md cross-references updated. - pyproject.toml + uv.lock: bump `0.0.5 → 0.0.6` to ship the cumulative audit cleanup (#73 + #75 + #77 + this PR). Closes audit findings #5, #8, #12, #13, #14, #15, #16 (reports/audit-2026-05-18.md). Breaking (pre-v0.1): callers of `IdentifiedVAR.forecast_error_variance_decomposition` must use `IdentifiedVAR.fevd` (no signature change). The long name was always an alias; per repo policy pre-v0.1 API breaks are in scope. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This branch was previously deployed
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
src/impulso/_ma.pywithcompute_ma_phi(A: list[np.ndarray], horizon: int). Numpy@broadcasts over leading dims, so the helper handles both the single-rotation(n, n)case used bySignRestrictionand the posterior-batched(C, D, n, n)case used byIdentifiedVAR— without branching._ma_coefficients(identified.py) and_check_restrictions_at_horizons(identification.py) each delegate tocompute_ma_phiexactly once. The pre-refactor in-line recursions are deleted._guard_no_time_dimlifts ontoVARResultBase. Each subclass declares a class-level_PRIMARY_KEY: ClassVar[str]("irf"/"fevd"); the base helper reads it. Removes ~30 lines of duplication.Closes #76. Third of four stacked PRs from
plans/2026-05-18-audit-cleanup-plan.md. Stacked on top of #75 (PR2).Test plan
uv run python -m pytest tests/test_ma.py -v— 10/10 pass (single-draw, batched, edge cases, byte-for-byte agreement with the pre-refactor recursion on a fixed seed, input validation).make test— 281/281 pass (+10 vs PR2: the newtest_ma.pysuite). No behavioural change visible to tests outsidetest_ma.py— pure refactor.make check— ruff + ty clean.grep -c compute_ma_phi src/impulso/identified.py src/impulso/identification.py— 2 each (1 import + 1 call); satisfies "exactly once per file".🤖 Generated with Claude Code