Repository navigation
fix: make naive and snaive forecasts match their definitions - #1611
Nicoló Angileri (nicoloangileri) wants to merge 13 commits into
Conversation
Naive fitted simple exponential smoothing with smoothing_level=0, so it forecast the estimated initial level (the training mean) instead of the last observation, the same as avg. SeasonalNaive used smoothing_level=1, so it forecast the last observation and ignored the tuned season. - Naive uses smoothing_level=1 and forecasts the last observation - SeasonalNaive fits a seasonal random walk, SARIMA(0,0,0)(0,1,0)[season], whose forecasts repeat the last season - SeasonalAverage reads the tuned season from its params when fit does not receive one
|
@microsoft-github-policy-service agree |
Li Jiang (thinkall)
left a comment
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Overall review of the complete PR: changes are required.
flaml/automl/time_series/ts_model.py:748-755: invalid seasonal lengths fabricate forecasts. Require a positive integer, reserve naive fallback forseason == 1, and reject folds with fewer thanseasontraining observations.flaml/automl/time_series/ts_model.py:336-342:predict(int)enriches the horizon before handling integers, so datasets with held-out test rows can shift the seasonal phase. Handle integer horizons first and forecast directly from the fitted training boundary.flaml/automl/time_series/ts_model.py:755-757: using SARIMAX for deterministic seasonal repetition is prohibitively expensive for long seasons and repeated CV fits. Cache the final seasonal cycle and implement tiling/slicing directly, including supported in-sample shifts.
Posted by thinkall-agent-auto-reviewer
- Require a positive integer season, keep the naive fallback for season 1 only, and reject training data shorter than one season - Start integer prediction horizons right after the fitted training data instead of after held-out test rows - Repeat the last training season directly instead of fitting SARIMAX, which was slow for long seasons, and predict in-sample values one season back
|
Thanks for the review. All three points are addressed in 327a744:
New tests cover each point: invalid seasons, too few observations, |
Li Jiang (thinkall)
left a comment
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Overall review of the complete current PR: one change is still required.
flaml/automl/time_series/ts_model.py:390-394,758-759: sparse in-sample requests are reduced to a start/end interval, andSeasonalRandomWalk.predict()returns every timestamp in that range. A request for only January 10 and January 12 therefore also returns January 11, breaking row-count and positional alignment. Select/reindex fitted values by the exact requested timestamps after validation, and add a sparse in-sample regression test.
The prior seasonal-length, integer-horizon phase, and SARIMAX performance blockers are resolved.
Posted by thinkall-agent-auto-reviewer
Statsmodels estimators predict every period between the first and last requested in-sample timestamps, so a sparse request such as January 10 and 12 also returned January 11. Select the requested timestamps from that range, validated the same way as future timestamps.
|
Addressed in 7a97eec. The in-sample test now also requests three sparse training dates. It fails on the previous commit (11 rows instead of 3) and passes now. |
Li Jiang (thinkall)
left a comment
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Overall review of the complete current PR: approved.
Sparse in-sample prediction now selects the exact requested timestamps and preserves cardinality, order, index, and values. The earlier seasonal-length validation, integer-horizon phase, and deterministic high-performance seasonal repetition fixes remain correct. Overall tests cover sparse/contiguous in-sample requests, delayed and partial future cycles, held-out test data, holdout/CV folds, timestamp validation, calendar indexes, exogenous model paths, shapes/dtypes, empty requests, and serialization.
No blocking issues found.
Posted by thinkall-agent-auto-reviewer
|
Hi Li Jiang (@thinkall), thanks again for the review. CI is green and there are no conflicts, but the PR still shows as blocked on my side. Is anything else needed from me before it can be merged? Happy to rebase or adjust if needed. |
|
Hi Li Jiang (@thinkall), a gentle follow-up: thanks for updating the branch with main last week. Is anything else needed from me before this can be merged? Happy to help. Thanks! |
There was a problem hiding this comment.
🟡 Changes recommended
Seasonal search spaces still permit configurations that cause AutoML fits to fail on short training partitions.
2 open issues
What changed in this PR
Corrects FLAML’s simple forecasting baselines and aligns shared statsmodels prediction behavior with training boundaries.
Changes:
- Makes naive forecasts repeat the last observation and seasonal naive forecasts repeat the last season.
- Honors tuned seasonal-average parameters and corrects integer-horizon and sparse in-sample predictions.
- Adds regression tests for forecast values, boundaries, and season validation.
| File | Description |
|---|---|
| test/automl/test_forecast.py | Adds baseline forecasting and validation regressions. |
| flaml/automl/time_series/ts_model.py | Corrects baseline models and shared prediction handling. |
Review effort: Balanced
- Limit the season search domain and its initial value to the training rows the search space sees (with holdout, the rows before validation) - Select the SeasonalAverage lag order up to the season only as far as the training rows allow, so short cross-validation folds no longer make ar_select_order raise and abort AutoML - Add regression tests for short series with holdout and cross-validation
With an odd number of training rows, (n - 1) // 2 lags still allowed a saturated AR model with no residual degrees of freedom, and statsmodels 0.14 raises ZeroDivisionError in AutoReg.fit. Cap the lag order at (n - 2) // 2 and test an odd-length series where order selection picks the largest lag.
Li Jiang (thinkall)
left a comment
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Overall review of the complete updated PR: no blocking code issue found; approval withheld because CI is not fully green.
The new season-domain bound avoids configurations longer than the available training history. The capped autoregressive lag order now preserves a residual degree of freedom on short histories. All seven focused short-series, lag-order, holdout and CV regressions pass locally, and the review-code Low harness is clean. The current Balanced Copilot review recommends approval, with no unresolved review threads. Three build checks remain failing, so the all-CI-passed approval gate is not met. This is a formal comment review, not an approval.
Posted by thinkall-agent-auto-reviewer
|
Hi Li Jiang (@thinkall), thanks for the review. The three red The Build workflow on Happy to rerun or rebase once |
Li Jiang (thinkall)
left a comment
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Overall review at 31c851c70cefd4e5684928d166d5962cb00cf4c3: completed the full six-lens Medium review-code assessment and independently checked the material findings. One P2 remains: seasonal-naive historical predictions silently replace an internal missing seasonal lag with the first training observation. This is reproducible through the public AutoML fit/predict API, not only by inspecting the new helper; details are inline.
Validation: python -m pytest test\automl\test_forecast.py -k 'simple_forecaster or naive or seasonal or average_forecasters or statsmodels' -q --disable-warnings passed (24 passed, 10 deselected). Additional real sparse-history checks across seven estimator families, with naive and New York timezone-aware dates, passed; speculative universal timezone/exogenous-index failures are not being posted. Green CI and a positive Balanced Copilot recommendation do not resolve the independently reproduced missing-lag defect, so approval is withheld.
A nonblocking clarification: the new lag-cap test comments still describe the earlier bounds; the current formula gives maxlag 4 for 11 rows and maxlag 1 for five rows. Both existing fixtures do detect removal of the cap.
Posted by thinkall-agent-auto-reviewer
Only the first season has no earlier value and falls back to the first observation; fillna also replaced every later missing lag with it. Fill the leading season only, add a regression with an internal missing target, and correct the lag-cap test comments to the current bounds.
Head branch was pushed to by a user without write access
Li Jiang (thinkall)
left a comment
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Reviewed the missing-seasonal-lag correction at 51e052c32655351529f32a1b065c8d81ea04c80c with review-code. The previously reported P2 is fixed: only the leading season is seeded with the first observation; later missing seasonal lags remain missing. Last-cycle storage and future forecasting are unchanged. The lag-cap comments now match the formula.
Validation: python -m pytest test\automl\test_forecast.py -k 'simple_forecaster or naive or seasonal or average_forecasters or statsmodels' -q --disable-warnings passed (25 passed, 10 deselected). Independent direct prediction returns [1.0, NaN, 5.0], and actual public AutoML prediction preserves the missing historical lag. The Low harness was CLEAN and its provenance was audited and committed locally. No issue found in this corrective delta.
This is not an approval: the actual current-head Build has not completed successfully. Approval remains subject to all same-head code, discussion, Balanced Copilot and workflow/check gates.
Posted by thinkall-agent-auto-reviewer
Li Jiang (thinkall)
left a comment
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Overall review of the complete current PR at 51e052c32655351529f32a1b065c8d81ea04c80c: approved.
I reassessed the full main-to-head production/test changes and connected shared statsmodels prediction paths, rather than treating the small corrective delta as whole-PR clearance. Naive and seasonal forecasts, training-boundary integer horizons, exact sparse historical selection, season validation, short-partition search bounds and autoregressive residual degrees of freedom remain consistent. The previously reported missing-lag P2 is fixed: only the leading season is seeded, while later missing seasonal lags remain missing.
Validation: python -m pytest test\automl\test_forecast.py -k 'simple_forecaster or naive or seasonal or average_forecasters or statsmodels' -q --disable-warnings passed (25 passed, 10 deselected). Validated unchanged-head review-code evidence was reused; it was not presented as a new model run. No blocking code issue found.
Fresh same-head gates confirm resolved review threads, an explicit Balanced Copilot approval recommendation, successful applicable check contexts, and successfully completed actual Build/docs/formatting workflows. The only conditional release-job skip is source-backed: gh-release excludes pull_request events.
Posted by thinkall-agent-auto-reviewer
Why are these changes needed?
Three of the simple time series forecasters do not forecast what their names say.
naivefitsSimpleExpSmoothingwithsmoothing_level=0.0, so the level never moves from the estimated initial level, which is the training mean. It forecasts the same values asavg.Naive.predict(int)also returnedparams["initial_level"]as the "last observation".snaiveusessmoothing_level=1.0, which gives the plain naive forecast: the last observation for every step, whateverseasonis.savgreadsseasonfrom thefitkwargs, but AutoML passes the tuned value through the estimator params, so the searchedseasonnever reachedar_select_orderandmaxlagstayed at 1.With the 30-point linear series from the #1587 regression test (10 to 20) and
period=3, and with an 84-day series with a weekly pattern andperiod=7:naive, linearnaive, weeklysnaive, weekly,season=7savg, weekly,season=7On the NYC energy data used in
test_extra_models.py, main givesnaivethe same MAPE asavg(0.1377), andsnaivethe MAPE thatnaivegets with this PR (0.1152).Changes:
Naiveusessmoothing_level=1.0, so it forecasts the last observation. Itspredictoverride is removed, and theStatsModelsEstimator.predictpath handles bothintand DataFrame inputs.SeasonalNaivekeeps the last training season and repeats it (SeasonalRandomWalk), so its forecasts repeat the last season exactly. Its in-sample predictions are the values one season earlier, and the first season uses the first observation. It requires a positive integerseason, falls back to the naive model only forseason == 1, and raises if the training data is shorter than one season.StatsModelsEstimator.predict(int)builds the forecast frame fromtrain_end_date, so an integer horizon starts right after the fitted training data. It used to start afterend_date, which is the end of the held-out test rows when a dataset has them. This also applies to ARIMA, SARIMAX and Holt-Winters.SeasonalAverageusesself.params["season"]whenfitdoes not receiveseason, so the existingfit(dataset, season=3)call still works.#1545 also touches
SeasonalNaive.predict([0]to.iloc[0]). This PR removes that method, so whichever lands second will need a small rebase.Related issue number
None.
Checks
The three new tests in
test/automl/test_forecast.pyfail on main and pass here.test/automl/test_forecast.py: 19 passed, 2 skipped. I also rannaive,snaive,savgandavgthroughAutoMLon the NYC energy data fromtest_extra_models.py, with exogenous regressors. All return 180 forecasts with no NaN, but I didn't run that module itself because it needs mlflow and Spark.