[FEAT] Add Horizon for fitted values#586
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 914d2cef46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33b2b1c8cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Thanks for the contribution — this addresses a long-standing gap (issue #346) and the direct model path ( Major1. Quadratic complexity in
|
…rollout and stronger validation/tests
nasaul
left a comment
There was a problem hiding this comment.
PR Review — Round 2
Great progress on the previous round of comments — all 6 items have been addressed. Two blocking issues remain before this can merge.
Bug — Static features crash forecast_fitted_values(h>1) [blocking]
Location: mlforecast/forecast.py:617–636
_compute_recursive_fitted_values_on_demand strips static columns from hist before calling temp_ts._fit(), but then passes static_features=self.ts.static_features to _fit. Inside TimeSeries._fit, it tries to extract the static columns from hist, which no longer has them, causing:
ValueError: Feature names seen at fit time, yet now missing: static_0
This affects any user who passes static features to fit() — including the default static_features=None which auto-detects all non-time/non-target columns as static.
The failing test tests/test_forecast.py::test_recursive_forecast_fitted_values_on_demand_h_with_static_features already covers this path and currently reproduces the crash.
Recommended fix: tell _fit there are no static columns to extract, then copy the already-computed static_features_ from the original TimeSeries:
temp_ts._fit(hist, ..., static_features=[id_col], ...)
temp_ts.static_features_ = self.ts.static_features_This works correctly because static features are still used during prediction. In TimeSeries.predict, the feature matrix is built by horizontally concatenating static_features_ with the lag/date features (see core.py:981). By copying self.ts.static_features_ after _fit, the correct static values — which are constants per series by definition — are present when temp_ts.predict(models=self.models_, ...) is called, so the models see exactly the same feature matrix they were trained on. The only thing we skip is redundantly re-extracting them from hist, which is what was crashing.
_fitted_train_df_ doubles memory footprint [blocking]
A deep copy of the full training DataFrame is stored in self._fitted_train_df_ indefinitely after fit(fitted=True). For large datasets this permanently doubles the memory footprint of the fitted object.
Required fix: accept the training data as an optional parameter to forecast_fitted_values() so users can opt out of the storage cost entirely:
def forecast_fitted_values(self, h=1, *, train_df=None): ...When train_df is provided, use it directly without storing a copy on self. When it is None, fall back to the cached _fitted_train_df_ for convenience. This keeps the current zero-friction API for small datasets while giving memory-constrained users a way to avoid the overhead entirely. The fit and forecast_fitted_values docstrings should document this trade-off.
Nit — Document why the per-origin loop was chosen over full vectorization
In the previous review I suggested a fully vectorized approach: preprocess all T origins into a single feature matrix of shape (T, n_features) and then do exactly h batched model calls updating lag columns in-place, reducing model evaluations from O(T²) down to O(h) regardless of dataset size. The original implementation was O(T²); the current one improves this to O(T·h) — one temp_ts.predict(horizon=h) per origin, each doing a full h-step autoregressive rollout — but stops short of the fully vectorized O(h) path.
The current approach may be intentional, but this reasoning isn't captured anywhere. Please add a comment above _compute_recursive_fitted_values_on_demand explaining the trade-off so future maintainers understand why the vectorized path was not taken.
Nit — Silent no-op in direct model path
In forecast_fitted_values (forecast.py:874–886), the h argument is silently ignored if "h" not in res.columns. An assert "h" in res.columns would close this silent failure path.
Summary: Two blockers before merge: fix the static features bug (test already failing) and add a train_df parameter to forecast_fitted_values() to avoid the memory doubling. All previous comments have been resolved.
|
We kept the current recursive execution model intentionally. MLForecast already vectorizes recursive forecasting across series at each forecast step, but remains iterative across horizon steps. A fully origin-batched fitted-values implementation would be a separate rollout engine that batches across training origins as an additional axis. That is a materially larger algorithmic change than this PR, because it would need to reproduce the exact current recursive semantics for lag updates, lag transforms, date features, static features, dynamic exogenous alignment, and target transforms. For this PR we prioritized correctness and parity with the existing predict path, and documented that trade-off rather than introducing a second, more specialized engine only for multi-step fitted values. |
There was a problem hiding this comment.
💡 Codex Review
Line 653 in 402fea4
MLForecast.forecast_fitted_values now returns an h column, but AutoMLForecast.forecast_fitted_values still merges per-model frames only on id/time. With multiple models this creates duplicated/suffixed horizon columns (h_x/h_y or similar), so the combined output no longer has a single reliable horizon column and can break downstream consumers that expect one h field per row.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2326ef6769
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
nasaul
left a comment
There was a problem hiding this comment.
Great work Jan, just added better docstring.
Allows users to set horizon for fitted values with param h. Standard is still h=1, but can be changed:
Works as expected.
Solves #346

Checklist: