Repository navigation
fix(ts): prevent overlapping AR and MA lags in SARIMAX configurations - #1661
Laasya priya Bhadri (laasyapriya2007) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The fallback reduces both AR and MA orders when only one seasonal component overlaps, incorrectly discarding valid terms from the non-overlapping component.
1 open issue
What changed in this PR
This PR addresses #845 by preventing overlapping seasonal and non-seasonal lags from interrupting FLAML’s SARIMAX forecasting.
Changes:
- Clamp overlapping AR and MA orders during configuration conversion.
- Add a targeted retry for overlapping-lag errors, with warning logging and parameter updates.
- Add regression tests for combined overlap, single-component overlap, and unchanged valid configurations.
| File | Description |
|---|---|
test/automl/test_forecast.py |
Tests lag clamping and successful fitting and forecasting. |
flaml/automl/time_series/ts_model.py |
Adds configuration clamping and defensive fit recovery. |
Review effort: Balanced
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Changes requested at 6d43bf0a128bbf6f755d3b538014832ce23f6c24: two independently reproduced P2 correctness issues.
-
Preserve valid sparse lag specifications.
SARIMAX(task="ts_forecast", p=[1,3], d=0, q=0, P=1, D=0, Q=0, s=12)now raisesTypeError: '>=' not supported between instances of 'list' and 'int'inconfig2params, although lags 1 and 3 do not overlap seasonal lag 12. The corresponding MA configuration (p=0, q=[1,3], P=0, Q=1, s=12) fails likewise. Both configurations actually fit and produce six finite predictions on exact main46bce0ce989aae0271e68e5daac5b8a9b0522b87in both native Windows Python 3.10 and 3.13 stacks. The new head fails during construction, before fitting. Handle scalar order counts and iterable lag specifications separately and add non-overlapping sparse-order regression coverage. -
Preserve the other valid order in the fallback. I independently confirmed Copilot's existing finding, without duplicating or resolving that thread. With post-init injected
(p,d,q)=(4,0,6)and seasonal(1,0,0,4), only AR overlaps, but the actual fitted backend order becomes(3,0,3)instead of retaining valid MA order 6. The MA-only analogue(6,0,4)/ seasonal(0,0,1,4)also fits(3,0,3)instead of(6,0,3). These are actual statsmodels construction/retry/fit results on both stacks, not simulated backend failures. The injection explicitly exercises the advertised defensive path; this is not a claim that normal built-in search necessarily reaches that path.
Validation: the intended scalar-overlap, targeted retry/logging, unrelated-error propagation, direct native fit/predict, repository regression and public AutoML control suite passes 16 cases per stack (32 total). The same test definitions on exact main give 7 expected feature-counterfactual failures and 9 passing controls per stack. The public AutoML starting-point case also passes on main, so it is a passing control, not proof that the invalid supplied trial was necessarily evaluated. The dedicated sparse-order probes add four current failures/four native main fit-predict controls; the fallback probes add four wrong-order reproductions/four prior unhandled-overlap controls. Original outputs and qualifications are retained; no contributor source was changed.
The planned Medium independent review-code execution remains blocked at its authenticated CLI prerequisite: no complementary reviewer, six-lens result or clean skill signoff is claimed. Driver-confirmed issues and current-head Balanced Copilot feedback are distinct evidence. No PR approval is submitted.
Posted by thinkall-agent-auto-reviewer
| # non-seasonal orders below the first seasonal lag to keep configs valid. | ||
| s = params.get("s", 0) | ||
| if s > 0: | ||
| if params.get("P", 0) > 0 and params.get("p", 0) >= s: |
There was a problem hiding this comment.
[P2] Keep valid sparse AR/MA lag lists working
These scalar comparisons reject existing valid statsmodels lag-list configs. For example, p=[1,3], q=0, P=1, D=0, Q=0, s=12, d=0 has no overlap, but construction now raises TypeError at p >= s; the MA analogue fails at q >= s. Both fit and produce six finite predictions on exact main in native Python 3.10/3.13 controls. Preserve iterable lag specifications rather than comparing them directly to the scalar seasonal period, with regression tests.
Posted by thinkall-agent-auto-reviewer
|
Thanks @thinkkll and Copilot for the reviews — both findings addressed in 3b13e2d. 1. Preserve the non-overlapping order in the fallback (both reviews) — agreed, this was a real flaw: the fallback clamped 2. Keep flexible (sparse) lag specifications valid — the scalar comparisons could raise Tests (extended Re: the earlier Build attempt failures (ubuntu 3.12/3.13) — the latest attempt of that run is green on all jobs; this push re-triggers CI. |
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Verified the corrective delta from 6d43bf0a128bbf6f755d3b538014832ce23f6c24 to 3b13e2d493c6a24d34c206826b74a33cfd3f16cf. Both specifically reported P2 issues are corrected in the tested paths: valid sparse AR/MA lag lists/tuples pass through without scalar comparison errors, and the defensive AR-only/MA-only fallback preserves the other valid order (6) instead of clamping both components to 3.
On native Windows Python 3.10 and 3.13, 17 cases pass per stack (34 total). The same current test definitions against the previous head give 9 expected failures and 8 passing controls per stack. Coverage includes actual sparse-list statsmodels fit/six-step prediction, actual post-init defensive fallback fitted-order checks, NumPy-integer/scalar normalization, retained endog/exog data, retry logging and unrelated ValueError propagation. The fallback injection is explicitly a defensive-path test, not a claim that normal built-in search must reach it. Original failures and the earlier changes request remain preserved.
This clears only those two driver-confirmed findings, not whole-feature or independent review-code signoff. The planned Medium complementary six-lens review remains blocked at the authenticated CLI prerequisite; no reviewer model or clean skill result is claimed. Broader sampled-season HPO and optional/version coverage are not cleared by these tests. No earlier review was dismissed and no foreign thread was resolved. Final approval still requires all fresh same-head source, thread, Balanced Copilot and applicable contexts/actual workflows to succeed.
Posted by thinkall-agent-auto-reviewer

Fixes #845
Problem
AutoML's
sarimaxlearner occasionally suggested configurations where a lag appears in both the seasonal and non-seasonal autoregressive or moving-average components. For example, the config from the issue ({"p": 4, "d": 0, "q": 5, "P": 1, "D": 3, "Q": 2, "s": 4}, e.g. via user-suppliedstarting_points) makes statsmodels raise:Non-seasonal AR lags are
{1..p}and seasonal AR lags are{s, 2s, ...}, so wheneverp >= s(withP > 0) lagsis duplicated. The same overlap exists on the MA side whenq >= swithQ > 0. The overlap can be injected by user-suppliedstarting_pointsor user-provided custom search spaces that bypass the built-in search-space bounds.Solution
1. Config clamping in
SARIMAX.config2params(flaml/automl/time_series/ts_model.py): the override calls the parent implementation, then, whens > 0, clampsptomax(0, s - 1)ifP > 0 and p >= s, and clampsqtomax(0, s - 1)ifQ > 0 and q >= s. This keeps every lag strictly below the first seasonal lag so no lag can be shared between the seasonal and non-seasonal polynomials, while leaving non-overlapping configs untouched.2. Defensive handling in
SARIMAX.fit(): model construction and fitting are wrapped in a targetedtry/except ValueError. Only the statsmodels overlapping-lag error (message containing"are in both the seasonal and non-seasonal") triggers the fallback; any otherValueErroris unrelated and propagates unchanged (nothing is silently hidden). On the fallback path a meaningful warning is logged describing the original order, the seasonal order and the error,self.paramsis updated to the actually-fitted (clamped) order so reported params are not misleading, and the model is refit once with the safe order.Tests
Added
test_sarimax_overlapping_lags_validation()intest/automl/test_forecast.py:p=4, q=5, P=1, Q=2, s=4— both AR and MA overlap) is clamped top=3, q=3and fits without the overlapping-lagValueError, producing a usable 6-step forecast;p>=s, P>0, Q=0) clampsponly; MA-only overlap (q>=s, Q>0, P=0) clampsqonly — both fit successfully;Actual results with
python -m pytest test/automl/test_forecast.py -k "sarimax" -q:(the warnings are pre-existing statsmodels
ValueWarning/ConvergenceWarningnotices on synthetic data)Additionally verified manually:
estimator_list=["sarimax"]with the invalidstarting_points) now completes:fit succeeded,best_estimator = sarimax;fit()fallback path handles invalid params injected after init and an unrelatedValueErrorstill propagates;