Skip to content

fix(automl): include latest observation in time-series CV - #1616

Open
林SO (Linxiushen) wants to merge 1 commit into
microsoft:mainfrom
Linxiushen:fix/time-series-cv-final-observation
Open

林SO (Linxiushen) wants to merge 1 commit into
microsoft:mainfrom
Linxiushen:fix/time-series-cv-final-observation

Conversation

@Linxiushen

Copy link
Copy Markdown

Why are these changes needed?

Time-series cross-validation currently places every fold one row too early and excludes the final observation from validation. cv_train_val_sets starts from len(train_data) - 1, although its validation slice has an exclusive end. Use the sample count as the exclusive endpoint so the last fold reaches the latest observation. This agrees with the TimeSeriesSplit configured by TimeSeriesTask and preserves custom cv_step_size spacing.

A public AutoML.fit reproduction with 59 zero-valued observations followed by a value of 100, the average forecaster, three folds and a five-step horizon reports MAE 0.0 before this change. It correctly reports 100 / 15 = 6.6666666667 afterward. The omitted observation can therefore affect model-selection scores.

The regressions compare folds against scikit-learn for two horizons and three index types, exercise overlapping and separated validation windows, and run the real AutoML/Statsmodels forecast-and-score path. All nine new cases fail on unchanged main.

Related issue number

No existing issue found for this boundary error.

Validation

  • New regressions against unchanged main: 9 failed, 3 existing tests passed.
  • python -m pytest test/automl/test_ts_data.py -q: 12 passed on Windows with both Python 3.12 / NumPy 2.5.3 / pandas 3.0.6 / scikit-learn 1.9.1 / Statsmodels 0.15.0 and Python 3.10 / NumPy 1.26.4 / pandas 2.3.3 / scikit-learn 1.7.2 / Statsmodels 0.14.6.
  • python -m pytest test/automl/test_forecast.py -k 'statsmodels or average_forecasters or simple_forecaster or log_training_metric' -q: 9 passed, 9 deselected.
  • python -m pre_commit run --all-files --show-diff-on-failure: all hooks passed.
  • An independent review reran the 12 data/CV tests successfully. Tests use real local estimators and files; the complete optional-backend suite was not run. Statsmodels emits existing frequency/index warnings.

Checks

  • I've used pre-commit to lint the changes in this PR.
  • No documentation changes are needed for this bug fix.
  • I've added regression tests corresponding to the changes.
  • I've made sure all hosted checks have passed (pending after submission).

Prepared with AI coding assistance; the fix and regressions were independently reviewed and executed locally.

@thinkall Li Jiang (thinkall) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall review of the complete PR: approved.

The exclusive CV endpoint now correctly includes the latest observation while preserving strict train-before-validation ordering, expanding-window behavior, custom step spacing, and parity with sklearn TimeSeriesSplit. Boundary and end-to-end coverage exercises small datasets, horizons, gaps/overlaps, fold counts, and leakage constraints. No blocking issues found.

Posted by thinkall-agent-auto-reviewer

This branch has not been deployed

No deployments
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