Skip to content

fix(automl): fix time_idx computation and support panel data in add_time_idx_col - #1641

Open
Atalaa (arhylsion) wants to merge 8 commits into
microsoft:mainfrom
arhylsion:fix/time-idx-panel-forecast
Open

Atalaa (arhylsion) wants to merge 8 commits into
microsoft:mainfrom
arhylsion:fix/time-idx-panel-forecast

Conversation

@arhylsion

Copy link
Copy Markdown

Why are these changes needed?

This pull request addresses a business logic defect in flaml.automl.data.add_time_idx_col(X):

  1. Fix operator precedence in index calculation:
    Previously, X["time_idx"] = timestamps - timestamps.min() / freq evaluated as timestamps - (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).

  2. Support panel datasets with repeated timestamps across groups:
    Previously, timestamps was calculated solely from unique_dates = X[TS_TIMESTAMP_COL].drop_duplicates(). When panel data contains multiple time series entities sharing identical timestamps, assigning unique_dates values caused missing index alignment and pandas.errors.IntCastingNaNError. The normalized index is now computed across all rows in X[TS_TIMESTAMP_COL].

  3. Handle short time-series edge cases & modernize pandas APIs:
    Guarded pd.infer_freq when len(unique_dates) < 3 to prevent ValueError: Need at least 3 dates to infer frequency, and replaced deprecated .view("int64") with .astype("int64").

Related issue number

Closes #1640

Checks

@arhylsion

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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

Comment thread flaml/automl/data.py Resolved

@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 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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Regular daily series crossing spring daylight saving time can still produce duplicate forecasting indices.

2 open issues

Review effort: Balanced

Comment thread flaml/automl/data.py Resolved
@arhylsion

Copy link
Copy Markdown
Author

Thank you Li Jiang (@thinkall) for the thorough review and catch!

I have updated add_time_idx_col so that whenever a regular frequency is inferred (including month-end ME/M and daily series spanning DST transitions), it maps observations to their positional index via pd.Index(unique_dates).get_indexer(...). This guarantees strictly consecutive step indices and properly preserves identical indices across panel groups, while retaining the existing MS and Y conventions as well as the elapsed-time division fallback for irregular inputs.

I have also added regression tests for both the month-end panel series and the DST transition panel series in test/automl/test_ts_data.py, and formatted the code with black and ruff to ensure all pre-commit checks pass.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused implementation and regression tests have no identified blocking issues; tests were not executed.

No open issues 🎉

2 resolved since last review

Review effort: Balanced

@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.

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

@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 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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The fallback can still assign identical indices to distinct timestamps through precision loss or irregular spacing.

2 open issues

Review effort: Balanced

Comment thread flaml/automl/data.py Resolved
Comment thread flaml/automl/data.py Resolved

@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.

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

@arhylsion

Copy link
Copy Markdown
Author

Thank you Li Jiang (@thinkall) and Copilot for pointing out these two edge cases!

I have addressed both issues in the latest commit:

  1. Preserve integer precision: Replaced Series.diff() with np.diff(unique_dates.astype("int64").to_numpy()) in the fallback to avoid floating-point casting and rounding errors. Nanosecond-resolution timestamps (e.g. 2021-01-01 and 2021-05-01 00:00:00.000000003) now correctly produce [0, 1] instead of collapsing to [0, 0].
  2. Prevent collapsing irregular intervals: Replaced the modal interval with the greatest common divisor (math.gcd(*diffs)) as the base step. For irregular dates such as January 1, 2, 4, and 6, this yields consecutive distinct step indices [0, 1, 3, 5] rather than [0, 0, 1, 2]. I have also added an explicit check rejecting spacings that are not integral multiples of the chosen base step.
  3. Added regression tests test_add_time_idx_col_nanosecond_precision and test_add_time_idx_col_irregular_spacing in test/automl/test_ts_data.py, and formatted with black.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Signed-int64 arithmetic still overflows for valid long-span timestamps, producing negative time indices.

1 open issue
2 resolved since last review

Review effort: Balanced

Comment thread flaml/automl/data.py
Comment on lines +262 to +270
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()

@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.

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-native int.

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

Comment thread flaml/automl/data.py
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)

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.

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

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.

[Bug]: Incorrect time_idx computation and IntCastingNaNError on panel data in add_time_idx_col

3 participants