[Reginald Alfret V (reginaldalfret)]

Fixes #1640

Summary

Fixes time_idx calculation and alignment in add_time_idx_col(X):

1. Operator Precedence: Corrected (timestamps - timestamps.min()) // step to ensure subtraction occurs before division, preventing large nanosecond timestamp values.
2. Panel Dataset Alignment: Mapped calculated integer indices across all rows of X[TS_TIMESTAMP_COL] using date_to_idx rather than assigning the unique_dates slice directly by index (which previously left duplicate timestamps in panel datasets as NaN and caused IntCastingNaNError).
3. Edge Cases & Modern Pandas Compatibility: Replaced deprecated/removed Series.view("int64") with .astype("int64") and safely handled datasets with fewer than 3 timestamps where pd.infer_freq cannot infer frequency.
4. Unit Tests: Added unit tests in test/automl/test_ts_data.py covering panel datasets and small timestamp counts.

[Copilot]

🟡 Changes recommended

Regular calendar frequencies can still produce duplicate time indices, affecting forecasting and validation splits.

1 open issue

• Regular-frequency timestamps receive incorrect duplicate time indices · New

What changed in this PR

Fixes time-index calculation and duplicate-timestamp alignment in FLAML’s forecasting data preparation, addressing #1640.

Changes:

• Corrects arithmetic and maps indices to every panel row.
• Handles small timestamp sets and replaces deprecated pandas usage.
• Adds panel-data and small-input regression tests.

File	Description
test/​automl/​test_ts_data.py	Adds panel and small-input tests.
flaml/​automl/​data.py	Updates time-index calculation, alignment, and frequency handling.

_{Review effort: Balanced}

[Li Jiang (thinkall)]

Overall review at 79b9032cdd9a22d6a83ac81791450543dc813fb7: changes requested for one P2 in the replacement time-index calculation.

The timestamp dictionary fixes panel-row alignment, and the two submitted panel/small-input regressions pass. However, a recognized regular frequency still falls through to fixed-duration division unless it is the existing month-start/year special case. Complete month-end and spring-DST series therefore receive duplicate or skipped model time steps despite satisfying the no-missing-timestamps assumption.

Independent shuffled three-group checks reproduce this on the actual helper: six month ends produce [0, 0, 1, 2, 4, 5] instead of [0, 1, 2, 3, 4, 5]; 24 month ends yield only 23 distinct indices per group, with 69/72 rows differing from ordinal steps. New York daily midnights across spring DST produce [0, 1, 2, 2, 3, 4], and weekly spring-DST dates also collide. The fall-DST control passes, and row order/timestamp/group values remain preserved, so this is specifically a frequency-to-index defect rather than a universal panel or timezone failure.

Please use calendar ordinal positions when frequency inference succeeds and retain appropriate short/irregular fallback behavior. Add complete month-end and timezone-aware daily panel regressions. This is an incomplete correction of touched helper logic, not a claim that released-main calendar indexing previously worked. The related #1641 addresses the same helper and may be useful when coordinating the fixes.

The independent Low review-code result and current Balanced Copilot both identify this P2. Referencing the existing discussion instead of adding a duplicate inline.

Validation: python -m pytest test\automl\test_ts_data.py -k add_time_idx_col -q --disable-warnings passed (2 passed, 12 deselected). The actual formatting workflow is failing; local full all-file pre-commit also detects the original extra EOF blank line in the submitted test file. That source-only formatting effect was restored exactly, and artifact-only checks passed; this is not reported as a successful full-source hook run. Approval remains withheld.

Posted by thinkall-agent-auto-reviewer

[Copilot]

🔵 Needs a closer look

The irregular-series fallback can assign the same time index to distinct timestamps.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Use minimum positive gap to preserve distinct irregular date indices

flaml/​automl/​data.py:267

For irregular dates Jan 1, Jan 3, Jan 5, and Jan 6, the modal gap is two days. The floor division below therefore produces [0, 1, 2, 2], assigning Jan 5 and Jan 6 the same time_idx. Use the minimum positive gap so distinct timestamps retain distinct indices, and add a regression test for this case; it should produce [0, 2, 4, 5].

1 resolved since last review

• Regular-frequency timestamps receive incorrect duplicate time indices

_{Review effort: Balanced}

[Li Jiang (thinkall)]

Reviewed the correction at 87f5937c52ee1bc1c21a593970fbe3b62824c795. The previously reported P2 for complete regular calendar/DST series is fixed: recognized frequencies now use ordinal positions rather than elapsed-duration floor division.

Four focused tests passed. Independent exact-prior/current comparisons on shuffled three-group panels eliminate all previous six/24-month-end, spring-daily-DST and weekly-DST mismatches; the former 69/72 extended-panel errors are now zero. Fall-DST controls, shared-date equality, original row/value order and integer output remain correct. Empty/one/two-date, irregular-gap and legacy month-start checks also pass. The Low review-code result is CLEAN for this corrective delta, with audited actual gpt-6.1-sol provenance. The former source EOF blank line is gone and full all-file pre-commit passes.

A separate P3 retained fallback limitation from the current Balanced Copilot review is independently confirmed: irregular Jan 1, Jan 3, Jan 5 and Jan 6 yield [0,1,2,2] under the modal two-day gap, in both the exact previous helper and this head. A minimum-positive-gap reference gives [0,2,4,5]. This is not introduced by the latest regular-frequency correction and is separate from the now-fixed complete-series P2; the helper's stated assumption and desired irregular-input semantics should be reconciled.

This is not an approval. Balanced Copilot currently says Needs a closer look despite its No open issues badge, so an explicit approval recommendation is absent. The retained fallback concern and all applicable current-head check/actual-workflow gates must be reconciled before overall approval. No other reviewer's thread was changed.

Posted by thinkall-agent-auto-reviewer

[Reginald Alfret V (reginaldalfret)]

Hi Li Jiang (@thinkall),

Thank you for the follow-up review on 87f5937.

I have updated the irregular-series fallback in add_time_idx_col to address the P3 limitation:

1. Minimum Positive Gap Fallback: When frequency inference returns None, the helper now uses pos_diffs.min() as the integer division step. For the irregular series 2021-01-01, 2021-01-03, 2021-01-05, 2021-01-06, this produces [0, 2, 4, 5] instead of [0, 1, 2, 2], ensuring all distinct timestamps receive distinct indices while preserving relative time-step proportions.
2. Preserved Regular Frequency Logic: Calendar ordinal indexing via pd.Index(unique_dates).get_indexer(...) remains active for recognized regular frequencies (month-ends, daylight saving transitions, etc.).
3. Regression Test: Added test_add_time_idx_col_irregular_gaps in test/automl/test_ts_data.py, verifying distinct index assignment, panel alignment, and shuffled row order.
4. Validation: All 5 targeted unit tests and all 16 tests in test_ts_data.py pass; black and ruff checks pass cleanly with compliant EOF formatting.

[Copilot]

🔵 Needs a closer look

The irregular-frequency fallback can distort timestamp spacing and assign duplicate indices through rounding.

No open issues 🎉

2 previously missed issues

In code that hasn't changed since last review

Avoid float precision loss when computing timestamp differences

flaml/​automl/​data.py:265

Series.diff() returns float64 differences for these int64 timestamps, which can round a gap upward. With nanosecond-resolution dates 2021-01-01 and 2022-01-01 00:00:00.000000003, the gap rounds from 31536000000000003 to 31536000000000004. Floor division then assigns both dates index 0 instead of [0, 1]. Compute adjacent differences in integer arithmetic and add a regression test for this precision loss.

Use GCD of timestamp gaps to preserve irregular spacing

flaml/​automl/​data.py:268

The minimum gap need not divide every timestamp offset. For Jan 1, Jan 3, Jan 6, and Jan 8, the two-day minimum and floor division produce [0, 1, 2, 3], hiding the longer middle interval. Use the greatest common divisor of the integer gaps to preserve spacing ([0, 2, 5, 7] here), and add a regression test with non-divisible gaps.

_{Review effort: Balanced}

[Li Jiang (thinkall)]

Corrective review at c5cc931c552f7c0ccff7a21704ed448b0245301e: fix verified; this is not an approval.

The previously confirmed modal-gap follow-up is fixed: Jan 1/3/5/6 now maps to [0,2,4,5], including shuffled three-group panels with duplicate DataFrame index labels. Five focused tests pass; independent 24-month, spring/weekly-DST and empty/short controls retain correct ordinal/shared-date behavior.

Two separate retained P3 fallback limits from the current Copilot observation were independently checked on both prior and current source. A year-plus-three-nanoseconds interval rounds upward in float64 Series.diff and gives [0,0]; Jan 1/3/6/8 gives [0,1,2,3] rather than a one-day GCD grid [0,2,5,7]. These behaviors are not introduced by this min-gap correction. Integer adjacent differences and a documented irregular grid convention are follow-ups; no new complete-regular-series P2 is claimed.

The Low review-code harness is CLEAN (actual gpt-6.1-sol), with audited input/model/read independence. Canonical evidence was committed locally after full all-file pre-commit passed. No contributor implementation was edited or pushed, and no other reviewer's thread was changed.

Approval is withheld: no explicit current-head Balanced Copilot approval recommendation; applicable check contexts are not all successful; actual Build: in_progress/None; retained irregular fallback precision/grid-spacing follow-ups are not reconciled.

Posted by thinkall-agent-auto-reviewer

[Copilot]

🔵 Needs a closer look

Fixed-width subtraction can still overflow for valid wide-range timestamps and produce negative indices.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Prevent int64 overflow in datetime difference and offset calculations

flaml/​automl/​data.py:278

The fallback still subtracts in signed int64 before converting to Python integers. For two valid datetime64[ns] dates, 1700-01-01 and 2200-01-01, np.diff overflows to a negative value and is discarded; the offset subtraction on line 278 also overflows, producing [0, -2668289673709551616] instead of [0, 1]. Offset subtraction can overflow even when each adjacent difference fits. Compute both differences and offsets with Python integers, then cast the normalized indices. Add regression coverage for both cases.

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

The fallback output dtype can overflow on supported Windows configurations despite passing the int64 range check.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Use int64 dtype to prevent Windows timestamp index overflow

flaml/​automl/​data.py:288

On Windows with supported NumPy 1.x (pyproject.toml:49), dtype=int is 32-bit, but the overflow guard checks against the int64 maximum. Irregular timestamps at midnight, midnight + 1 microsecond, and midnight + 1 hour produce indices [0, 1, 3600000000]. These pass the guard but exceed the output dtype's range, causing the conversion to fail. Use np.int64 explicitly so the output matches the checked range.

_{Review effort: Balanced}

[Li Jiang (thinkall)]

Review at 1255b64c702cd82bbb9dd39576f02e0efb3c69a0: changes requested for one remaining supported-platform P2.

The Python-integer adjacent/offset arithmetic fixes the original 500-year nanosecond counterexample, preserves earlier GCD/calendar/DST cases and rejects unrepresentable indices explicitly. Eight focused repository tests passed. Independent exact-boundary checks accept [0,1,int64_max] and correctly raise for an index of 2*int64_max. The Low review-code result is issues-found; its original Low repository coverage recommendation is retained separately from the P2 below.

P2 - Make the output dtype match the checked range. The guard uses np.iinfo(np.int64).max, but the final np.array(..., dtype=int) is 32-bit on supported Windows/NumPy1.x configurations. The repository's test dependency explicitly includes NumPy<2 for Windows/Python<3.13. Ordinary irregular timestamps at midnight, midnight+1 microsecond and midnight+1 hour require [0,1,3600000000]. That index fits int64 but not the output's int32 range.

Use explicit np.int64 for the output array so the successful range check guarantees representability. Add the representability and output-dtype regression cases recommended by the reviewer. This is a remaining supported-platform range-contract defect, not a claim that the latest arithmetic repair regressed a previously correct Windows/NumPy1.x path.

Evidence: current NumPy2/int64 execution returns the expected indices; an isolated final-int32-output substitution cannot represent them. A real NumPy1.x Windows runtime was not executed, so the simulation's OverflowError is not claimed as that version's precise failure mode (older versions can wrap). The official NumPy2 migration guide documents the default-int change from C long to intp. The representability mismatch is independent of whether a particular version wraps or raises.

Referencing the existing current Copilot observation rather than creating a duplicate inline. Approval remains withheld.

Canonical evidence was committed at the exact inspected source parent after full all-file hooks passed. Historical b017 evidence is preserved separately and no stale review is posted for it. No contributor implementation was edited or pushed.

Posted by thinkall-agent-auto-reviewer