Repository navigation
fix(automl): fix time_idx computation and support panel data in add_time_idx_col - #1641
Atalaa (arhylsion) wants to merge 8 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
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Re-reviewed commits 79257166c635de982cb063a8ec7386f8e7b8e292 -> 3f4e1600c4a317179b666325557a67e44ca43e8b. The only delta is main's website/yarn.lock update of follow-redirects 1.16.0 -> 1.16.1. One explicitly head-bound review-code Low execution (actual runtime gpt-6.1-sol) found no new issue in this identical inherited patch. Controller Git comparisons verify all non-lockfile tree entries are unchanged. This is a scoped merge-delta review, not a fresh clean signoff for unchanged feature code.
Version/URL/caret/SRI shape checks pass. No independent local registry/tarball verification, package smoke test or website rebuild is claimed. Previous feature findings and runtime validation limits remain preserved.
At the 2026-10-26T12:13:44.943946+00:00 gate snapshot, actual current-head workflows were: Code formatting completed/success; Build in_progress/no conclusion; docs completed/success. Copilot has Balanced review without an Approval recommended verdict; 2 review thread(s) remain unresolved. Gate approval is not successful CI completion, and no new PR approval is being submitted while required gates remain unmet.
Posted by thinkall-agent-auto-reviewer
…lsion/FLAML into fix/time-idx-panel-forecast
|
Thank you Li Jiang (@thinkall) and Copilot for pointing out these two edge cases! I have addressed both issues in the latest commit:
|
| unique_ts = unique_dates.astype("int64").to_numpy() | ||
| diffs = np.diff(unique_ts) | ||
| if len(diffs) > 0: | ||
| freq_val = math.gcd(*[int(d) for d in diffs]) | ||
| if freq_val <= 0: | ||
| freq_val = 1 | ||
| else: | ||
| freq_val = 1 | ||
| all_timestamps = X[TS_TIMESTAMP_COL].astype("int64").to_numpy() |
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Changes requested after reviewing the late update at exact head aecd54e5c4dd64dd9e9aca74377205db60fd93da since 3f4e1600c4a317179b666325557a67e44ca43e8b.
The intended precision and irregular-grid corrections check out: exact prior/current probes fix the supplied nanosecond and daily examples. The selected repository tests passed 6 cases on each of two existing runtimes (12 passing executions, not 12 unique cases). Independent review-code Low execution with actual gpt-6.1-sol found two remaining P2s:
- Valid wide-span timestamps overflow before GCD. Explicit
datetime64[ns]dates 1700-01-01 and 2100-01-01 map[0, 1]on exact prior code but[0, -1]current on both NumPy 1.26 and 2.3. This corroborates the existing Copilot concern; I am not duplicating that inline or resolving its owner thread. Convert to Python integers before subtraction, then range-check normalized values. - Windows NumPy 1.x output width corrupts the finer GCD grid, detailed inline: required indices
[0, 1, 3600000001, 7200000001]become negative int32 values. Use explicit int64 output with bounds validation rather than platform-nativeint.
The boundary evidence executes the exact repository helper with native Windows pandas/NumPy, not a dtype shim or a full forecasting pipeline. Stable versions were Python 3.10.19/NumPy 1.26.4/pandas 2.3.3; modern versions were Python 3.13.5/NumPy 2.3.3/pandas 3.0.5. No dependencies or contributor source were changed. The complete supported matrix is not claimed.
The current failures are distinguished from the old modal-spacing collapse rather than being presented as previously working irregular behavior. The wide-span case is a verified new regression; the platform-width case remains an incomplete part of the numeric correction. Passing CI does not clear either source finding.
Posted by thinkall-agent-auto-reviewer
| f"Timestamp intervals in '{TS_TIMESTAMP_COL}' are not integral multiples " | ||
| f"of the base step ({freq_val})." | ||
| ) | ||
| X["time_idx"] = (diff_from_min // freq_val).astype(int) |
There was a problem hiding this comment.
P2: store an explicit wide integer result; native int wraps this valid finer grid on supported Windows NumPy 1.x.
I executed this exact helper on Windows Python 3.10.19 / NumPy 1.26.4 / pandas 2.3.3, where np.dtype(int).itemsize == 4. Starting at 2021-01-01, use nanosecond offsets [0, 1000, 3600000001000, 7200000001000]. GCD is 1,000 ns, so normalized indices should be [0, 1, 3600000001, 7200000001]. This cast instead returns [0, 1, -694967295, -1389934591], dtype int32. The same source on NumPy 2.3.3's int64 default returns the correct values.
This is native execution, not a simulated dtype. Prior modal behavior also had collapsed positions [0, 0, 1, 2], so I am not claiming the previous irregular mapping was correct or that NumPy just changed its Windows width. Please range-check the computed indices and use explicit np.int64; add exact-value/dtype coverage for this ordinary two-hour microsecond grid.
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