Fix Rotbaum to handle short series #3065
Conversation
|
Thanks @leica2023! I think it would make sense to add a small test that reproduces the issue, and that would fail before this fix. More at a high level: in this PR, the |
|
Also fix the styling issue by applying |
| @@ -454,7 +454,7 @@ def make_features(self, time_series: Dict, starting_index: int) -> List: | |||
| prefix = [None] * abs(starting_index) | |||
| else: | |||
| prefix = [] | |||
| time_series_window = time_series["target"][starting_index:end_index] | |||
| time_series_window = time_series["target"] if prefix else time_series["target"][starting_index:end_index] | |||
There was a problem hiding this comment.
I would include this in the previous if else above
There was a problem hiding this comment.
Sure, makes sense.
Correct. The issue happens at prediction time for instances that have history length shorter than context_length. This method is also used in training data preparation, but this part ensures ts is long enough to contribute to training samples. |
|
@leica2023 need to also format the test scripts: |
| @@ -45,7 +45,7 @@ | |||
| "import os\n", | |||
| "import pprint\n", | |||
| "import random\n", | |||
| "from scipy import stats \n", | |||
There was a problem hiding this comment.
@leica2023 could you revert the changes to notebooks? No need to run black on them
| freq=freq, | ||
| ) | ||
|
|
||
| predictor = TreePredictor( |
There was a problem hiding this comment.
TreePredictor also needs importing?
There was a problem hiding this comment.
Added this and reverted ipynb styling.
| @@ -59,3 +61,69 @@ def test_rotbaum_smoke(datasets): | |||
| predictor = estimator.train(dataset_train) | |||
| forecasts = list(predictor.predict(dataset_test)) | |||
| assert len(forecasts) == len(dataset_test) | |||
|
|
|||
|
|
|||
| def test_short_history_item_pred(): | |||
There was a problem hiding this comment.
@leica2023 would this test fail before the fix contained in the PR? (That is, would it fail if run on the current dev branch)
lostella
changed the title
lostella
added
pending v0.13.x backport
lostella
removed
pending v0.13.x backport
Issue #, if available:
Description of changes:
for items with less than forecast_horizon historical steps, jobs failed due to error 'Number of features of the model must match the input. Model n_features_ is 88 and input n_features is 60'. This PR fixes this error.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Please tag this pr with at least one of these labels to make our release process faster: BREAKING, new feature, bug fix, other change, dev setup