feat: use common syntaxis and improve speed - #2
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a pre-processed “panel” representation of the input DataFrame to reduce repeated per-model/per-call data processing, and refactors several forecast implementations to reuse that shared panel and common quantile-column assignment utilities.
Changes:
- Add
PanelData+process_panel_from_df()and updateTimeSeriesDatasetto build from a pre-processed panel (with lazy tensor materialization). - Thread an optional
panelargument through multiple modelforecast()implementations andMultiModelForecasterMixinto reuse shared preprocessing work. - Refactor quantile column assignment and several joins/aggregations to use
utilsforecasthelpers; add/adjust tests for new dataset/panel behavior and quantile→level mapping.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_foundation_forecast.py | Updates test forecaster stubs to accept the new optional panel kwarg. |
| tests/helpers.py | Updates helper forecasters to accept panel for compatibility with new calling conventions. |
| tests/core/test_utils.py | Adds tests for process_panel_from_df() and TimeSeriesDataset.from_panel() behavior (order, values, laziness). |
| tests/core/test_forecaster.py | Updates expected behavior for quantile-derived level (excluding median-only / median level). |
| foundationforecast/models/toto.py | Accepts optional panel, uses shared dataset builder and shared quantile assignment helper. |
| foundationforecast/models/tirex.py | Accepts optional panel, uses shared dataset builder and shared quantile assignment helper. |
| foundationforecast/models/timesfm.py | Accepts optional panel (currently unused in one path) and uses shared dataset builder in v2 path. |
| foundationforecast/models/timegpt.py | Accepts optional panel (unused) to align with common forecast signature. |
| foundationforecast/models/tabpfn.py | Accepts optional panel (unused) to align with common forecast signature. |
| foundationforecast/models/t0.py | Accepts optional panel and uses shared dataset builder. |
| foundationforecast/models/sundial.py | Accepts optional panel, uses shared dataset builder and shared quantile assignment helper. |
| foundationforecast/models/patchtst_fm.py | Accepts optional panel, uses shared dataset builder and shared quantile assignment helper. |
| foundationforecast/models/flowstate.py | Accepts optional panel, uses shared dataset builder and shared quantile assignment helper. |
| foundationforecast/models/chronos.py | Accepts optional panel, reuses it for finetuning inputs, uses shared dataset builder and quantile assignment helper. |
| foundationforecast/core/utils.py | Adds PanelData, process_panel_from_df, grouped_std_by_id, and refactors TimeSeriesDataset to operate on panel arrays with lazy tensors. |
| foundationforecast/core/multi_model.py | Precomputes panel once per multi-model forecast call; switches merge to utilsforecast.processing.join. |
| foundationforecast/core/gluonts_forecaster.py | Vectorizes forecast→DataFrame conversion using panel ordering and batched column assignment. |
| foundationforecast/core/forecaster.py | Adds shared helpers for dataset construction and quantile-column assignment; uses faster utilsforecast aggregations/joins; adjusts quantile→level mapping logic. |
| foundationforecast/core/init.py | Exposes PanelData, process_panel_from_df, and grouped_std_by_id from the core package. |
Suppressed comments (1)
foundationforecast/core/forecaster.py:179
panelis being passed throughMultiModelForecasterMixinand added to many concreteforecastimplementations, but the baseForecaster.forecastsignature doesn’t accept it. This makes the public API inconsistent and breaks static typing / interface expectations. Consider addingpanel: PanelData | None = Noneto the base method signature as well.
def forecast(
self,
df: pd.DataFrame,
h: int,
freq: str | None = None,
level: list[int | float] | None = None,
quantiles: list[float] | None = None,
) -> pd.DataFrame:
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (3)
foundationforecast/core/forecaster.py:176
- The base
Forecaster.forecastsignature does not accept the newpanelargument, butMultiModelForecasterMixinnow passespanelto models and several built-in models have been updated to accept it. This makes the core interface inconsistent and can break customForecastersubclasses when used viaMultiModelForecasterMixin(unexpectedpanelkwarg) and also causes type-checking signature mismatches.
def forecast(
self,
df: pd.DataFrame,
h: int,
freq: str | None = None,
foundationforecast/models/chronos.py:345
- When
finetuning_configis enabled andpanelis not provided, the code will compute the panel twice: once inside_make_timeseries_dataset(..., panel=None)(viaTimeSeriesDataset.from_df) and again inside_maybe_finetune(..., panel=None)(via_build_fit_inputs_from_df). Computingpanelonce here and reusing it avoids duplicate preprocessing and aligns with the PR's speed goals.
freq = self._maybe_infer_freq(df, freq)
qc = QuantileConverter(level=level, quantiles=quantiles)
dataset = self._make_timeseries_dataset(
df,
batch_size=self.batch_size,
foundationforecast/models/timesfm.py:201
panelis already used below (passed into_make_timeseries_dataset(..., panel=panel)), so this_ = panelline is redundant and adds an unnecessary executed statement inside the hot path.
_ = panel
|
Heads up: I opened #18 as a refreshed copy of this PR on top of current |
this pr normalizes syntaxis and improves speed