You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix: compute time_idx in steps for non-monthly panel data - #1636
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
I've used pre-commit to lint the changes in this PR (note the same in integrated in our CI checks).
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.
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.
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.
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.
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.
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.
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.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The irregular-data fallback still produces incorrect time indices for uneven gaps and daylight saving transitions.
No open issues 🎉
2 previously missed issues
In code that hasn't changed since last review
Use a GCD base step to preserve irregular timestamp gaps
flaml/automl/data.py:271
For irregular dates such as Jan 1, 3, 5, and 6, frequency inference returns None and the modal gap is two days. The floor division then produces [0, 1, 2, 2], assigning the same time_idx to two distinct timestamps within a series instead of preserving gaps. Derive a base step that divides every observed delta (for example, their greatest common divisor), and add a regression where a shorter gap occurs less often than the modal gap.
Preserve calendar-day indices across DST transitions
flaml/automl/data.py:271
For local midnights on March 11, 12, 14, 15, 16, and 17, 2021 in America/New_York, frequency inference fails and the UTC-based fallback produces [0, 1, 3, 3, 4, 5] instead of calendar-day indices [0, 1, 3, 4, 5, 6]. Changing the modal interval to a greatest common divisor prevents collisions but still measures hours rather than calendar days. Preserve calendar-day steps for irregular daily input without changing elapsed-time semantics for subdaily input, and add a missing-date regression across DST.
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the complete PR at c88d7e0, including the new wall-clock fallback.
The previous spring-business/DST case is fixed: all six focused repository tests pass in each of four existing environments (pandas 1.5.3, 2.3.3 and 3.0.5), all 60 regular-calendar probe evaluations pass, and the existing period=4 predicate now selects four business observations per group.
One new P2 regression is detailed inline. A genuinely missing elapsed hour during the fall DST fold is preserved by the exact previous helper but compressed away by the current fallback. The paired preservation probe fails on the new source in all four environments. Please keep the business-day correction while validating or narrowing wall-clock inference so irregular subdaily grids retain their gaps.
The independent Low review returned issues_found. Native validation used the real current helper and exact prior-helper controls; no TFT training, public AutoML fitting, dependency changes or full minimum-dependency matrix is claimed. Existing irregular modal-gap/calendar-day limitations were not reposted as newly introduced by this commit.
The reason will be displayed to describe this comment to others. Learn more.
Re-reviewed commits c88d7e0ef1067a05f9bea4306b3155a8f7a43d96 -> 4dc11355d4b0f52660098c01fab79c53bb0605ab. The only delta is main's website/yarn.lock update of follow-redirects1.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.
Changes remain required. The existing P2 fall-DST regression remains: removing the actual repeated 01:00-05:00 hour from the New York hourly grid should preserve [0, 1, 3, 4, 5, 6], but the wall-clock fallback compresses it to [0, 1, 2, 3, 4, 5]. That exact prior/current paired failure was reproduced in all four existing environments before this merge. The helper is byte-unchanged, so this is retained evidence, not a claim of four new runs. The original finding is not resolved by this dependency update.
At the 2026-10-26T12:13:44.943946+00:00 gate snapshot, actual current-head workflows were: docscompleted/success; Code formattingcompleted/success; Buildin_progress/no conclusion. 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.
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the complete current PR at 597e23191befaa789b63b80faca940bf3adcc9e7, including the latest calendar/GCD correction.
The previous missing-fold-hour finding is fixed: the new source preserves [0,1,3,4,5,6], and the exact previous helper fails that regression. I verified the correction on both native stacks and resolved only my corresponding owned thread.
Two P2 issues remain, detailed inline: deleting one month-end observation changes month-relative steps into day-relative steps, and the wall-clock grid validator can allocate millions of candidate timestamps for only three observations.
Validation: all nine repository helper cases pass on each of pandas 2.3.3/NumPy 1.26.4 and pandas 3.0.5/NumPy 2.3.3 (18 executions, nine distinct cases). Another 73 repeated/shuffled panel fixtures per stack and nine unit/GCD/duplicate-index controls per stack pass. The separate exact prior/current missing-month counterexample and real bounded allocation witness establish the two findings despite that selected suite passing.
Whole-feature review-code effort was Medium: six qualified independent grok-4.7/xhigh lenses. Seven actual executions and all original results are retained in locally committed canonical evidence; one initial lens was excluded after its grep exposed sibling feedback and was independently rerun in a fresh source-only checkout. Neither excluded feedback nor a scoped correction is presented as independent clean signoff.
This is helper/panel-index validation, not estimator fitting or TFT training. No full minimum-dependency matrix, native Spark, pandas 3.1 execution, giant nanosecond-grid allocation or actual OOM is claimed. Historical version-compatibility and short-series allegations were adjudicated against concrete controls rather than automatically reposted.
At 2026-10-30T16:41:50.130084+00:00, actual current-head pull_request workflows were: docs completed/success; Code formatting completed/success; Build completed/success. Current Balanced Copilot review 5714447225 recommends approval. These external gates do not clear the two independently verified source findings, so changes remain required.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The helper assigns distinct daylight-saving fold observations the same time_idx because it does not require strictly increasing month ordinals before bypassing the elapsed-time fallback.
The reason will be displayed to describe this comment to others. Learn more.
Re-reviewed the correction from 597e23191befaa789b63b80faca940bf3adcc9e7 to f4c65a4349354907269129f339e4e8039018af01, and composed it with the retained whole-feature review rather than treating a scoped correction as blanket clearance.
Both previous P2s are fixed. Sparse month/quarter cases now retain calendar steps, and the grid validator builds at most one candidate per observation. A real nanosecond fold-gap probe now creates exactly three entries (24 bytes), with no giant allocation. Only my two corresponding signed threads have been resolved after committed native fix proof.
Changes remain required for the existing Copilot finding; I am corroborating it instead of posting another inline. New York instants 2021-10-07T01:30-04:00, 2021-11-07T01:30-04:00, and 2021-11-07T01:30-05:00 are distinct. The exact previous helper returns [0,744,745] per panel group, but the new calendar candidate returns [0,1,1], merging the two fold observations. I reproduced this with the real current helper and exact prior controls on both native stacks. Reject calendar candidates with non-increasing month ordinals so these inputs reach the elapsed-time fallback, and add the repeated/shuffled-panel regression.
Validation: all 12 repository cases pass on each of pandas 2.3.3/NumPy 1.26.4 and pandas 3.0.5/NumPy 2.3.3 (24 executions, 12 distinct cases); another 97 panel/calendar/unit fixtures pass per stack (194 evaluations). Exact prior controls give nine passes and three expected failures per stack, proving the targeted corrections. The separate fold counterexample still fails the timestamp-to-index uniqueness invariant on current source.
review-code Low used the actual CLI-default gpt-6.1-sol and returned issues_found. Its canonical evidence is committed locally on this exact author source. The previous whole-feature Medium's six qualified independent lenses, excluded initial result and validation limits are retained; this is not six new votes or a new compatibility guarantee. No estimator fitting, TFT training, native Spark, pandas 3.1 execution, full minimum-dependency matrix, huge allocation or OOM is claimed.
At 2026-10-30T21:15:36.291172+00:00, actual current-head pull_request workflows were: Build completed/success; docs completed/success; Code formatting completed/success. Current Balanced Copilot review 5718569371 says Changes recommended. The verified collision and its existing Copilot thread remain blockers; no PR approval is being submitted.
The reason will be displayed to describe this comment to others. Learn more.
Completed an overall review of the current PR at 0fe1cfdfb33c50056e6150e96af94130ea1b7a65, composing the complete current source with the retained whole-feature review and the new corrective evidence. No actionable issue remains in that qualified assessment.
The duplicate-month/fold finding is fixed. Both exact prior failing panels now preserve distinct timestamps: [0,1,1] becomes [0,744,745], and [0,0,1,2] becomes [0,1,721,1465]. Repeated and shuffled panel rows are covered. The helper rejects non-increasing month ordinals and reaches the existing elapsed-time fallback rather than merging fold observations. Sparse month/quarter and the earlier daily/business-day/DST and bounded-grid corrections remain intact.
Native validation on Windows pandas 2.3.3/NumPy 1.26.4 and pandas 3.0.5/NumPy 2.3.3: all 14 repository cases pass per stack (28 executions, 14 distinct cases), plus 97 regular/irregular/calendar/unit fixtures per stack (194 evaluations). Identical final repository cases with the exact previous helper give 12 passes and two expected failures per stack. The real nanosecond grid probe still creates three entries (24 bytes); no huge allocation or OOM was attempted.
The actual CLI-default gpt-6.1-sol Low corrective review returned clean; its canonical evidence is committed locally on this exact source parent. This is not six renewed whole-feature votes: the previous six qualified independent Medium lenses, excluded initial result, later corrective history and driver whole-source composition are retained with their original outcomes. The prior Copilot thread was resolved externally, not by this automation.
This is qualified native helper/panel-index validation, not a claim of public AutoML fitting, TFT training, native Spark, pandas 3.1 execution, the full minimum-dependency matrix, or universal compatibility beyond the inspected paths.
Fresh same-head gate check at 2026-10-31T01:50:45.317653+00:00: docscompleted/success (attempt 1); Code formattingcompleted/success (attempt 1); Buildcompleted/success (attempt 1). All review threads are resolved, current Balanced Copilot review 5720361750 explicitly recommends approval, all applicable contexts are successful, and all actual applicable workflow runs completed successfully. The gh-release job is verified PR-inapplicable at the unchanged workflow blob; its skipped result is not counted as passed validation. Approving the complete PR with the validation qualifications above.
## Summary
Merge public main through 386ff40 into the existing consolidation branch.
Resolve five conflicts while preserving upstream seasonal forecasts, training-log
append behavior, datetime preprocessing, portfolio isolation, search-space fixes,
and the public dependency policy.
Address introduced and tightly coupled review findings: use the fitted calendar
for integer forecasts, reject malformed prediction grids before backend calls,
measure forecast validation transforms without charging training history, retain
nullable and wide time indexes with bounded-memory gcd calculation, revalidate
Spark casts on the full column, and keep public SearchState AutoFE disabled by
default. Strengthen append/non-resume, missing-class, lag-bound, exported-input,
and conversion regression coverage.
## Prompting Intent
The engineer asked to use review-code on PR #1545, make needed changes, push the
existing upstream branch, and make the code ready for merging. Preserve ordinary
public installs/defaults, enable Fabric behavior explicitly or through local
runtime detection, and do not merge or bypass human review approval.
## Linked Sources
- Current PR and discussion: #1545
- Public main: 386ff40
- Seasonal forecast definitions: #1611
- Safe training-log append: #1615
- Panel time-index steps: #1636
- Missing datetime preprocessing: #1650
- Pandas preprocessing compatibility: #1648
- Fresh review decision and dispositions: reviews/pr-1545/task-1545-attempt-2-stage-attempt-1-review-1-grok-4.7.md
- All six fresh lenses and two focused verifications: reviews/pr-1545/task-1545-attempt-2-*.md
- Runtime/API contracts: flaml/fabric/README.md
## Rationale
The PR head was unchanged and previously CI-green, but current main introduced
16 focused commits and new conflicts. A fresh Medium follow-up used the current
catalog's complementary grok-4.7 reviewer at xhigh reasoning for six concurrent
lenses, then independent correctness and coverage verification.
Share partition transformation logic so forecast timing reflects held-out work
without changing transformed output or training-only operations. Preserve public
calendar and lag semantics rather than retaining obsolete PR-only expectations.
Keep threshold-based coercion explicit, but prevent a biased Spark sample from
choosing a cast unsupported by the full column.
All eight skill-owned reports ship with the changes and retain their original
findings and verdicts. Current-base AST comparisons and dependency probes
distinguish inherited concerns and unsubstantiated claims from merge regressions;
no clean reviewer sign-off or unrelated lifecycle/sampling redesign is claimed.
This is maintenance of an existing GitHub PR with no supplied ADO TaskId; its
existing infrastructure labels and GitHub sources provide the work linkage.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b0e4942d-f489-4b42-8a90-097ede4e88f7
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
add_time_idx_col(used forts_forecast_panelwhen the data has notime_idxcolumn, and in the TFT predict path) is broken for any frequency other than monthly/yearly:timestamps - timestamps.min() / freqonly divides the minimum, sotime_idxends up as the raw nanosecond timestamp. For daily data starting 2024-01-01 the first value is 1704067199999980288 instead of 0.X, so every row whose timestamp repeats (any panel with more than one series) gets NaN, and.astype("int")raisesIntCastingNaNError.Series.viewwas removed in pandas 3, so there it fails withAttributeErrorbefore any of that.This computes
(timestamp - first timestamp) // stepfor every row instead.Related issue number
None, found while reading the time series code.
Checks
test/automl/test_add_time_idx_col.pyfails on main (pandas 2.3 and 3.0) and passes with the change.