Skip to content

arima: fix coef stderr - #1206

Draft
José Morales (jmoralez) wants to merge 11 commits into
Nixtla:mainfrom
jmoralez:arima-coef-var
Draft

José Morales (jmoralez) wants to merge 11 commits into
Nixtla:mainfrom
jmoralez:arima-coef-var

Conversation

@jmoralez

Copy link
Copy Markdown
Member

No description provided.

@codspeed

codspeed Bot commented Aug 11, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 38.74%

❌ 2 regressed benchmarks
✅ 36 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
test_efficiency[AutoRegressive] 343 ms 821.4 ms -58.24%
test_efficiency[ARIMA] 8 ms 8.9 ms -10.13%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing jmoralez:arima-coef-var (5ccc79d) with main (1734b18)

Open in CodSpeed

@nasaul

Copy link
Copy Markdown
Contributor

Review: the negative variances, the m3 change, and the shared root cause

All of it traces back to one thing: mod is mutated in place by the objective and never fully restored, so the ML objective is not a function of its parameters.

1. What gets mutated through minimize

Args bound into the new partials: x, coef, mask, arma, mod, ncxreg, xreg, narma, trans.

Variable Mutated by the objective?
mod Yes — in place, and not fully restored
x No (x = x.copy() first), but mutated in the enclosing scope: x -= xreg @ coef[...] (arima.py:686, arima.py:885)
coef No (par = coef.copy()), but mutated in enclosing scope (coef[mask] = res.x, maInvert, then rebound by arima_undopars)
init Mutated in place at arima.py:713 — and it is the caller's array when the user passes init=
mask, arma, xreg, ncxreg, narma, trans Read-only

upARIMA (arima.py:226) writes mod["phi"], mod["theta"], mod["T"][:p,0], mod["Pn"][:r,:r], mod["a"][:] = 0 and returns the same dict. arima_like then passes mod["a"], mod["P"].ravel(), mod["Pn"].ravel() — all views — into C++, which writes through them (src/arima.cpp:405, src/arima.cpp:412).

upARIMA restores only a and the top-left r×r of Pn. The diffuse block Pn[r:, r:] (set to kappa = 1e6 in make_arima) is never restored, and the filter overwrites it. For (2,1,1)(0,1,0)[12]:

Pn diffuse diag BEFORE any eval : [1e6, 1e6, 1e6, 1e6]
eval 1 (fresh mod)              : 2.4804968998826187
Pn diffuse diag AFTER           : [-1.3e-19, 6.8e-17, 1.8e-16, -2.0e-16]
eval 2 (same mod)               : 2.8883488337137626
eval 3 (same mod)               : 2.8883488337137560
eval 4 (fresh mod again)        : 2.4804968998826187

The first evaluation differs from every subsequent one by 0.41. This is pre-existing on main; this PR makes it observable.

2. Why the variance goes negative

from statsforecast.models import ARIMA
from statsforecast.utils import AirPassengers as ap
m = ARIMA(order=(2, 1, 1), seasonal_order=(0, 1, 0), season_length=12).fit(ap).model_
  1. BFGS returns nit=0, status=2 ("precision loss") — it never leaves the CSS warm start, because f(x₀) = 2.4805 (fresh mod) and f(x₀+h) = 2.8883 (polluted mod), so the apparent gradient is ~0.41/h and the line search fails immediately.
  2. The PR computes a genuine numerical Hessian at that non-stationary point. It is indefinite, and stably so across step sizes — not an epsilon artifact:
eig(H, eps=1e-3) : [-1.86028, -0.02483, +0.73600]
eig(H, eps=1e-2) : [-1.86251, -0.02482, +0.73562]
eig(H, eps=1e-4) : [-1.86041, -0.02480, +0.73603]
  1. cho_factor(res.hess) raises LinAlgError (negative leading minor), the except falls through to lu_factor/lu_solve, which solves happily with an indefinite matrix → var diag = [0.00767, 0.00777, -0.00397]sqrtnan.

On main this was hidden rather than fixed: BFGS's hess_inv is PSD by construction, and with nit=0 it is exactly the identity, so main returns var = AᵀA/n_used = [0.00621, 0.00712, 0.00763] — non-negative and equally meaningless.

Coefficients and AIC are identical on both branches (aic = 1029.652); only the variance differs.

3. Why the m3 benchmark moved (4.46 → 4.33)

checkarima (arima.py:980):

return any(np.isnan(np.sqrt(np.diag(obj["var_coef"]))))

and myarima (arima.py:1157): if minroot < 1.01 or checkarima(fit): fit["ic"] = math.inf.

Negative variance → NaN SE → candidate silently dropped from the model search. Measured on AirPassengers:

PR   : selected ARIMA(1,1,0)(0,1,0)[12]  aic 1020.394  candidates 10  rejected 1
main : selected ARIMA(1,1,0)(0,1,0)[12]  aic 1020.394  candidates 10  rejected 0

The rejected candidate is exactly (2,1,1)(0,1,0)[12]. Same final pick on this series, but across the 3003 m3 series this changes which models survive selection. So the benchmark number did not move because the math got better — it moved because models started being discarded. (I did not re-run m3 itself, only established the mechanism.)

Recommendations

  1. Fix upARIMA to fully restore mod — all of Pn including the diffuse kappa block, plus P — or rebuild mod per evaluation. This is the root cause. It makes the objective a pure function of the parameters and lets BFGS actually converge; without it the numerical Hessian is measuring a discontinuous function.
  2. Don't silently fall back to LU. If cho_factor fails, the Hessian is not PD, so the point is not a minimum. Returning NaN (as the C++ path already does, 7781777) or warning is more honest than inverting an indefinite matrix.
  3. The T / skew-normal / GED branches are a regression. approx_hess3(...)[:k,:k] followed by inversion gives H_θθ⁻¹, but the marginal covariance is [H⁻¹]_θθ = (H_θθ − H_θψ H_ψψ⁻¹ H_ψθ)⁻¹. Dropping the Schur correction understates the ARMA standard errors by ignoring uncertainty in σ²/ν/α/β. The previous hess_inv[:k,:k] was the block of the inverse, which was correct in that respect.
  4. Minor:
    • np.isnan(res.hess).any() does not catch inf.
    • Hardcoded epsilon=1e-3 is an absolute step; questionable for xreg/intercept coefficients of order 100+.
    • The CSS pre-fit at arima.py:701 still uses args= while everything else moved to partial.

Conflicts in python/statsforecast/arima.py and tests/test_arima.py:

- upARIMA: kept main's cached, non-mutating `Z` dict, which supersedes this
  branch's `mod = {**mod}` copies (and drops the unused `T` update).
- ML/CSS optimizations: kept this branch's `partial`-based `obj_fn` +
  `approx_hess3` Hessian, layered onto main's `ml_obj`/`dist_tail_fit`
  bookkeeping (`args=ml_args` dropped since the objective is now bound).
- maInvert re-score: kept main's direct re-evaluation instead of re-running
  `minimize`, carrying `res.hess` through.
- Fell back to NaN when `cho_factor` finds the Hessian isn't positive
  definite, matching the existing singular-matrix handling; main's
  near-degenerate distribution tests hit this.
- tests: kept both sides' new tests.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_0119VbLrxzmn89AWrseL74fn
Three fixes on top of the numerical Hessian:

- Re-estimate the Hessian after maInvert re-parameterises the MA part, as
  R's optim(maxit = 0, hessian = TRUE) does. The Jacobian `A` was already
  evaluated at the re-parameterised coefficients while the Hessian came from
  the pre-inversion fit point. On AirPassengers ARIMA(2,1,1)(0,1,0)[12] this
  moves ma1's standard error from 0.03027 to 0.02919 (R: 0.02920); ar1/ar2
  and the ML fit now match R to 5 decimals too.

- Marginalise the distribution parameters out of var_coef. The t/skew-normal
  /ged branches sliced the arma block out of the Hessian and inverted that,
  giving variances conditional on sigma2 and the shape parameter. Inverting
  first and slicing after gives the marginal ones; the difference is under
  1% for t and ged but understates the intercept by ~99% for skew-normal,
  whose alpha is strongly correlated with the mean.

- Fold the three copies of the invert-or-NaN logic into `_coef_var`. The
  transformed branch used cho_factor, which additionally rejects Hessians
  that are merely indefinite and raises ValueError rather than LinAlgError
  on NaN input, so it needed a separate isnan pre-check and still crashed on
  the near-degenerate distribution fits.

test_coef_stderr now pins R's values (atol 2e-3 -> 1e-4) and covers ML as
well as CSS-ML; every one of these tests fails without the changes above.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_0119VbLrxzmn89AWrseL74fn
4.36 was recorded during the intermediate states of this branch, where
_coef_var used cho_factor: that also rejected merely-indefinite Hessians and
raised ValueError, which myarima swallows into ic = inf, so candidates were
dropped and 17 of the 174 Other series reselected.

Switching to np.linalg.inv dropped that. The search still rejects 4
candidates main keeps, but none of them win their series: the selected model
and AICc are identical to main for all 174, and the forecasts are
bit-identical. Both main and HEAD measure 4.4868, which the test's 1% rtol
does not admit against 4.36.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_019YXn1fursQmqpMazreQccZ
The parallel path runs every worker through GroupedArray's
_single_threaded_* wrappers, which cap BLAS at one thread. The serial
branches called GroupedArray directly, so they never got that cap.

It costs little until something makes LAPACK calls in the fitting loop. The
numerical Hessian adds ~1235 np.linalg.inv calls per M3 Other run on
matrices of size <= 8, and LAPACK's getri hands those to threaded BLAS3,
where waking the pool costs far more than the arithmetic.

Routing the six serial branches through the wrappers that already exist cuts
this branch's overhead on that run from +7.8% to +5.0% against main idle, and
from +22.4% to +12.9% with the other cores busy. It also stops the timings
swinging with machine load: run-to-run spread drops from ~1s to under 0.05s.
Forecasts are bit-identical.

Only numpy's OpenBLAS is capped -- the module-level ThreadpoolController
snapshots at import, before scipy's copy loads -- but capping scipy's too
measured no gain here, so that is left alone.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_019YXn1fursQmqpMazreQccZ
auto_arima_f raised "No suitable ARIMA model found" whenever the xreg was
rank-deficient -- two constant columns, which is what a static feature becomes
on a per-series model, and what generate_series feeds the dask/spark flow
tests. Every candidate carries the same xreg, so every candidate was rejected.

np.linalg.inv does not raise on a numerically singular matrix; it returns a
covariance with negative diagonal entries, whose square root is NaN, which is
exactly what checkarima rejects on. Main never saw this because scipy's BFGS
hess_inv is positive definite by construction, so the degeneracy stayed hidden
until the Hessian became a real one.

Decompose instead and read the spectrum. approx_hess3's central differences
resolve the Hessian to about sqrt(eps), so eigenvalues under that are
indistinguishable from zero: one below -tol is a genuine saddle and still
yields NaN, while the flat ones are directions collinear xreg leaves
unidentified and are dropped, inverting over the identified subspace. A
well-conditioned Hessian keeps every direction and the result is the plain
inverse, so the standard errors pinned against R are untouched.

This is a guard, not a cure -- a single constant regressor is equally
unidentified and stays undetected, since the design matrix is full rank and
only the likelihood is flat. Dropping collinear regressors up front is filed
separately.

eigh also raises LinAlgError on a Hessian holding NaN, which the previous code
only caught for inv.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_019YXn1fursQmqpMazreQccZ
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants