Repository navigation
Fix reversed R² inputs in time-series estimator scores - #1654
Li Jiang (thinkall) merged 1 commit into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are reported for the default time-series R² scoring correction.
No open issues 🎉
What changed in this PR
Corrects the default R² calculation in time-series estimator scoring by passing reference values before predictions, so the score uses reference variance.
Changes:
- Fixes the argument order without changing explicit-metric loss behavior.
- Adds synthetic
SeasonalNaiveregression tests for DataFrame andTimeSeriesDatasetinputs, including constant-reference, perfect-forecast, and explicit-metric controls.
| File | Description |
|---|---|
test/automl/test_time_series_score.py |
Adds regression tests and controls for time-series scoring. |
flaml/automl/time_series/ts_model.py |
Corrects the default r2_score argument order. |
Review effort: Balanced
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Review cycle 20261103-1941; exact source a55ffc314b5213c105785f4394c4238b69734f35.
Completed an overall review of the entire two-file R2 correction and regression tests with review-code (Low, actual gpt-6.1-sol) plus independent native validation. No actionable issue was found in the proposed change: default R2 now uses reference variance, while explicit R2/MAE loss conventions remain unchanged.
Local command on each native Windows interpreter:
python -m pytest -q --import-mode=importlib -p no:cacheprovider --color=no test\automl\test_time_series_score.py
Result: 8 passed on Python3.10.19/NumPy1.26.4 and 8 passed on Python3.13.5/NumPy2.3.3. The same tests against exact prior main produced 2 expected failures and 6 passing controls per stack: the old default returns -2.0 instead of 0.4 for both direct and dataset inputs. Actual public import roots were verified; no baseline source was changed.
Review evidence is committed locally as 821b28b5e6e45348d562a7ecd9d5aaa1605981a5; pre-commit run --all-files passed before that evidence-only commit. Contributor source was not edited or pushed.
Fresh current-head gates pass: all review threads resolved, Balanced Copilot approval recommended, all applicable check contexts successful, and actual current-head Build/docs/formatting workflow runs completed successfully. The PR release job is intentionally skipped, not a passed deployment.
This signoff covers the proposed R2 fix, not unrelated existing main-branch findings or an unexecuted full forecast/Spark/TFT/GPU/performance/minimum-version matrix.
Posted by thinkall-agent-auto-reviewer
## Summary Merge public main through 46bce0c into PR #1545 and resolve the version conflict using the exact OSS 2.8.0 version file, not the internal post-release version. Preserve the upstream R2 target-order correction and inline Spark backend isolation. Audit the newer internal main and port its missing nullable/DST and delayed seasonal-forecast regressions. Refine the panel test gate to skip only the demonstrated legacy dependency combination, retaining stronger public prediction assertions and isolated artifact paths. ## Prompting Intent The engineer asked to update PR #1545 because FLAML Internal has new commits. They explicitly required test/automl/test_rouge_metric.py to keep the OSS version: the internal test was simplified only because some internal environments do not have rouge installed. ## Linked Sources - Consolidation PR: #1545 - Public synchronization target: 46bce0c - Public release update: #1658 - Spark backend correction: #1657 - Time-series R2 correction: #1654 - Complete OSS ROUGE test: https://github.com/microsoft/FLAML/blob/46bce0ce989aae0271e68e5daac5b8a9b0522b87/test/automl/test_rouge_metric.py - Internal main audited through 7d7ab67afb8bb453a7783898e6a4f42a99b616ff ## Rationale Most new internal library changes are public-main synchronization or are already implemented more completely in this branch. Merge public history and selectively carry the missing regressions rather than merging unrelated internal Git history or overwriting public runtime contracts. Keep the real rouge_score Score/AggregateScore coverage unchanged instead of copying the internal SimpleNamespace workaround. Preserve public step-index semantics, estimated smoothing initialization, optional integration defaults, and existing quality checks. Exclude internal release tooling, delivery pipelines, Conda/certificate changes, timeout-only policy, and the .post1 suffix. This is maintenance of the existing GitHub PR with no supplied ADO TaskId. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b0e4942d-f489-4b42-8a90-097ede4e88f7
Why are these changes needed?
TimeSeriesEstimator.score()passes predictions asy_truetosklearn.metrics.r2_score()when no metric is specified. R² is not symmetric: this uses prediction variance as its denominator and reports an incorrect default score. For example, predictions[7, 9, 7, 9]against references[6, 10, 8, 12]score-2.0instead of0.4.Pass the references first. Add native
SeasonalNaiveregressions for DataFrame andTimeSeriesDatasetinputs, with constant-reference, perfect-forecast and explicit-metric controls.AutoML.score()delegates to the estimator's score; the training objective is outside this change.Related issue number
None; found while inspecting the time-series scoring path.
Checks
pre-commit run --all-filesand the configured hooks on both changed files; all passed.git diff --checkandpip checkpassed.Validation used Windows/Python 3.12, NumPy 1.26.4, pandas 2.2.3, SciPy 1.14.1 and scikit-learn 1.6.1. Tests use the local seasonal random-walk forecaster and synthetic data, with no downloads or external services. No real-model, GPU or performance claim.
AI assistance disclosure: Codex developed the fix and tests, and a separate agent reviewed the exact change and evidence.