Skip to content

fix: compute time_idx in steps for non-monthly panel data - #1636

Open
Anish Mehta (anishmehta24) wants to merge 7 commits into
microsoft:mainfrom
anishmehta24:fix/ts-panel-time-idx
Open

Anish Mehta (anishmehta24) wants to merge 7 commits into
microsoft:mainfrom
anishmehta24:fix/ts-panel-time-idx

Conversation

@anishmehta24

Copy link
Copy Markdown
Contributor

Why are these changes needed?

add_time_idx_col (used for ts_forecast_panel when the data has no time_idx column, and in the TFT predict path) is broken for any frequency other than monthly/yearly:

  • timestamps - timestamps.min() / freq only divides the minimum, so time_idx ends up as the raw nanosecond timestamp. For daily data starting 2024-01-01 the first value is 1704067199999980288 instead of 0.
  • The values are computed from the unique dates and assigned back to X, so every row whose timestamp repeats (any panel with more than one series) gets NaN, and .astype("int") raises IntCastingNaNError.
  • Series.view was removed in pandas 3, so there it fails with AttributeError before any of that.

This computes (timestamp - first timestamp) // step for every row instead.

Related issue number

None, found while reading the time series code.

Checks

test/automl/test_add_time_idx_col.py fails on main (pandas 2.3 and 3.0) and passes with the change.

add_time_idx_col divided only the minimum timestamp by the step
(operator precedence), so time_idx held raw nanosecond values, and it
assigned the per-unique-date series back to X, which left NaN for any
repeated timestamp in panel data and crashed the int cast. It also
used Series.view, which pandas 3 removed.

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 fix addresses the reported defects with regression coverage and no identified blocking issues.

No open issues 🎉

What changed in this PR

Fixes time-step indexing for non-monthly/yearly panel forecasting data.

Changes:

  • Computes integer time steps for every row, including repeated timestamps.
  • Replaces removed pandas Series.view usage with astype.
  • Adds regression tests for daily data and shared panel timestamps.
File Description
test/​automl/​test_add_time_idx_col.py Tests daily step counts and repeated timestamps.
flaml/​automl/​data.py Corrects timestamp arithmetic and row alignment.

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.

Overall review of the complete PR: no blocking code issue found; approval withheld pending CI.

The corrected subtraction/division order computes elapsed time steps, and assigning from the original timestamp column preserves repeated panel dates and row-index alignment. Both focused regression tests passed locally. The review-code harness is clean and Copilot's current Balanced review recommends approval with no open issues. The current head still has failing build checks, so the all-CI-passed approval gate is not met. This review does not approve the PR for merge.

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.

🟢 Approval recommended

The focused fix and regression tests address the reported failures with no identified blocking issues.

No open issues 🎉

Review effort: Balanced

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 fix addresses the reported failures with regression coverage and no unresolved findings.

No open issues 🎉

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.

Overall review at c99d0484e355d7df37ffbfd8fa0060e133ebab21: the ordinary daily/panel correction is useful, but one P2 correctness issue remains in the new step mapping. Complete regular month-end and timezone-aware daily series still receive duplicate indices and artificial gaps; the inline review includes independently executed reproductions.

Validation: python -m pytest test\automl\test_add_time_idx_col.py -q --disable-warnings passed (2 passed). Separate repeated-panel calendar/DST fixtures fail the consecutive-step invariant, and the review-code whole-PR Low harness independently identified the same root cause. Green CI and a positive Copilot recommendation do not resolve this source defect, so approval is withheld. No contributor code was modified.

Posted by thinkall-agent-auto-reviewer

Comment thread flaml/automl/data.py Outdated

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 fix and regression tests address the reported failures, with no blocking issues identified; tests were not executed during review.

No open issues 🎉

Review effort: Balanced

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

Irregular daily data still loses steps across DST, and the month-end test uses an alias unavailable in older supported pandas versions.

1 open issue
1 previously missed issue

In code that hasn't changed since last review

Medium severity DST causes irregular daily data to lose calendar-day spacing

flaml/​automl/​data.py:266

The irregular fallback still loses calendar-day steps across DST. For New York midnights on March 12, 13, 14, 15, and 17, 2021, infer_freq returns None. The modal interval is 24 hours, so this division produces [0, 1, 2, 2, 4] instead of [0, 1, 2, 3, 5]. Distinct dates then share a time_idx. Preserve calendar-day spacing for irregular daily data without changing elapsed-time indexing for intraday data, and add a regression combining DST with a missing date.

Review effort: Balanced

Comment thread test/automl/test_add_time_idx_col.py Outdated

@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 e796511b5c07e38efe7a190063396202aa98f934 with review-code (Low, gpt-6.1-sol). The complete regular month-end/DST mapping is repaired on modern pandas, but changes are requested for supported-version compatibility.

[P2] Keep the new month-end fixture within the declared pandas range. Both AutoML and test extras permit pandas>=1.1.4, while the new freq="ME" fixture requires pandas 2.2+. I reproduced ValueError: Invalid frequency: ME natively with Python 3.10/pandas 1.5.3. pd.offsets.MonthEnd() produces the same six dates on pandas 1.5.3, 2.3.3 and 3.0.5. This corroborates the existing Copilot finding; no duplicate inline is being added.

The actual focused file reports 5 passed on pandas 3.0.5, but 2 failed, 3 passed on pandas 1.5.3. The second failure is the timezone test: the unchanged pd.infer_freq(Series) call rejects timezone-aware Series on that runtime. Exact prior and current code both show that TypeError, so I am not attributing it to the new ordinal branch. The same dates wrapped in a DatetimeIndex infer D; please ensure the newly added DST regression also runs across the declared pandas range.

Fifteen native calendar/DST/permutation controls verify the modern regular-grid fix and original panel-row/index preservation. Existing irregular modal-gap/DST limitations are recorded separately and are not claimed fixed or introduced by this correction. I have not resolved my older signed calendar/DST thread while supported-version handling remains incomplete, and no other reviewer's thread was touched.

No contributor implementation/test or installed package was changed. Approval remains withheld.

Posted by thinkall-agent-auto-reviewer

@anishmehta24

Copy link
Copy Markdown
Contributor Author

Li Jiang (@thinkall) fixed in 0755530. The test uses pd.offsets.MonthEnd(), and infer_freq now gets a DatetimeIndex, so the tz-aware DST case also works on old pandas. All 5 tests pass on pandas 1.5.3 (Python 3.10) and 3.0.6.

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 address the reported failures, with no unresolved findings from static review.

No open issues 🎉

1 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 complete PR at 0755530, including the older-pandas correction.

The portable MonthEnd fixture and DatetimeIndex inference fix the two concrete compatibility cases previously reported. The focused file now passes all five tests in each of four existing environments: Python 3.10/pandas 1.5.3, Python 3.10 and 3.12/pandas 2.3.3, and Python 3.13/pandas 3.0.5.

One residual P2 is detailed inline: a complete spring-DST business-day panel still gets artificial step gaps on pandas 1.5.3. Extended native probes passed 59 of 60 regular-calendar evaluations; the failing case makes the existing four-step panel holdout predicate select only three observations per group. This is an incomplete supported-range calendar correction, not a claim that earlier legacy heads handled that input successfully. The already-known irregular modal-gap limitation is not being reposted as a newly introduced defect.

Validation used the real helper and the existing holdout predicate; it did not train TFT or claim a full minimum-dependency matrix. No contributor source or tests were edited.

Posted by thinkall-agent-auto-reviewer

Comment thread flaml/automl/data.py
unique_dates = X[TS_TIMESTAMP_COL].drop_duplicates().sort_values(ascending=True)
# assume no missing timestamps
freq = pd.infer_freq(unique_dates)
freq = pd.infer_freq(pd.DatetimeIndex(unique_dates))

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 - Preserve business-calendar steps when legacy timezone-aware inference fails. On supported-range pandas 1.5.3, pd.date_range("2021-03-10", periods=6, freq="B", tz="America/New_York") is a complete six-business-day grid, but this timezone-aware DatetimeIndex infers None (the same local wall-clock dates infer B). The real current helper therefore takes elapsed-time fallback and returns [0,1,2,4,5,6] for each repeated panel group, instead of [0,1,2,3,4,5].

These artificial gaps are observable downstream: with period=4, the existing max(time_idx)-period / time_idx>cutoff holdout rule selects only three observations per group. Please retain calendar-relative ordinal mapping for this legacy spring-business/DST case and add a cross-version regression, without changing genuinely irregular timestamp semantics or unconditionally discarding timezone information.

Exact earlier public helper controls raised TypeError on this legacy input. This is a remaining incomplete calendar correction, not a regression from previously working legacy behavior. Native helper/predicate validation was performed; no TFT training is claimed.

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.

3 participants