Repository navigation
fix(automl): fix time_idx computation and support panel data in add_time_idx_col - #1641
Atalaa (arhylsion) wants to merge 5 commits into
Conversation
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
🟡 Changes recommended
Regular month-end data still produces duplicate and skipped time indices.
1 open issue
What changed in this PR
Fixes time-index generation in FLAML’s forecasting data preparation, addressing #1640.
Changes:
- Corrects normalization and assigns indices across all panel rows.
- Handles fewer than three unique timestamps and replaces deprecated pandas conversion.
- Adds panel-data and two-timestamp regression tests.
| File | Description |
|---|---|
| test/automl/test_ts_data.py | Adds regression tests for panel and short-series inputs. |
| flaml/automl/data.py | Updates timestamp conversion, frequency handling, and index calculation. |
Review effort: Balanced
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Overall review at e828a00120de509935ec854f84d5f7a5683ad061: the panel and short-input fixes are useful, but one P2 correctness issue remains.
P2 - Fixed elapsed-time division collapses distinct regular calendar observations (flaml/automl/data.py:257-266). The fallback uses the modal epoch duration even when the inferred frequency is a variable-duration calendar frequency. Complete regular inputs with no missing timestamps reproduce:
| Input | Inferred frequency | Actual time_idx |
Expected relative steps |
|---|---|---|---|
| Six month ends from 2021-01-31 | ME |
[0,0,1,2,4,5] |
[0,1,2,3,4,5] |
Six America/New_York local days from 2021-03-12, crossing spring DST |
D |
[0,1,2,2,3,4] |
[0,1,2,3,4,5] |
Distinct observations receive duplicate indices and artificial gaps. TFT uses these indices for training cutoffs, encoder windows, and decoder construction, so valid forecasting input can fail dataset validation or select incorrect windows. Use calendar/ordinal steps rather than a fixed epoch duration, preserving equal indices for repeated panel timestamps and the intentional monthly/yearly conventions. Please add month-end and local-day DST regressions, including repeated panel rows.
Balanced Copilot independently identified the same root cause in the existing discussion; this review references that thread instead of duplicating an inline comment.
Validation: python -m pytest test\automl\test_ts_data.py -k add_time_idx_col -q --disable-warnings passed (2 passed, 12 deselected). Independent execution of the two regular fixtures above fails the consecutive-step invariant. The review-code Low harness also reported this issue. Separately, pre-commit run --all-files fails on the original contributor snapshot because test/automl/test_ts_data.py has an extra blank line at EOF; evidence-only hooks passed, not the original full tree. Contributor code was not modified. Approval is withheld for the unresolved correctness issue and incomplete/failing CI.
Posted by thinkall-agent-auto-reviewer
…lsion/FLAML into fix/time-idx-panel-forecast
|
Thank you Li Jiang (@thinkall) for the thorough review and catch! I have updated I have also added regression tests for both the month-end panel series and the DST transition panel series in |
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Reviewed the calendar-ordinal correction at 79257166c635de982cb063a8ec7386f8e7b8e292 with review-code. The previous calendar/DST P2 is fixed: recognized regular frequencies map sorted unique timestamps to ordinal steps, preserving row order and equal indices for shared panel dates. The existing month-start/year branches and irregular/short-input fallback remain unchanged. No issue found in this corrective delta.
Validation: python -m pytest test\automl\test_ts_data.py -k add_time_idx_col -q --disable-warnings passed (4 passed, 12 deselected). Independent shuffled three-group panels cover 24 month ends, spring DST, fall DST and weekly DST; all produce the expected ordinal mapping without modifying timestamp/group values. The Low harness was CLEAN; its exact-head provenance was audited and committed locally, with full all-file pre-commit passing on this source.
This is not an approval: the actual current-head Build has not completed successfully. Approval still requires complete same-head CI/check, discussion, Balanced Copilot and overall code-review gates.
Posted by thinkall-agent-auto-reviewer
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Overall review of the complete current PR at 79257166c635de982cb063a8ec7386f8e7b8e292: approved.
I reread the entire helper and test changes against main, including original panel-row alignment and the later calendar correction. Mapping regular timestamps to sorted unique-date ordinals preserves original row order and shared panel-date indices without month-end or DST collisions. Existing month-start/year conventions and the short/irregular modal-difference fallback remain intact. The earlier calendar/DST P2 is resolved, and no blocking code issue remains.
Validation: python -m pytest test\automl\test_ts_data.py -k add_time_idx_col -q --disable-warnings passed (4 passed, 12 deselected). The unchanged exact-head review-code artifact and independent 24-month/multi-group, spring/fall/weekly-DST evidence were verified and reused, not relabeled as a new model invocation.
Fresh same-head gates confirm all discussions resolved, an explicit Balanced Copilot approval recommendation, all applicable checks successful, and actual Build/docs/formatting workflows completed successfully. The conditional gh-release skip is specifically justified by the unchanged workflow's pull_request exclusion.
Posted by thinkall-agent-auto-reviewer

Why are these changes needed?
This pull request addresses a business logic defect in
flaml.automl.data.add_time_idx_col(X):Fix operator precedence in index calculation:
Previously,
X["time_idx"] = timestamps - timestamps.min() / freqevaluated astimestamps - (timestamps.min() / freq)due to Python operator precedence, producing large nanosecond numbers instead of normalized sequential integer indices (0, 1, 2, ...). It is now corrected to((all_timestamps - min_ts) // freq_val).astype(int).Support panel datasets with repeated timestamps across groups:
Previously,
timestampswas calculated solely fromunique_dates = X[TS_TIMESTAMP_COL].drop_duplicates(). When panel data contains multiple time series entities sharing identical timestamps, assigningunique_datesvalues caused missing index alignment andpandas.errors.IntCastingNaNError. The normalized index is now computed across all rows inX[TS_TIMESTAMP_COL].Handle short time-series edge cases & modernize pandas APIs:
Guarded
pd.infer_freqwhenlen(unique_dates) < 3to preventValueError: Need at least 3 dates to infer frequency, and replaced deprecated.view("int64")with.astype("int64").Related issue number
Closes #1640
Checks