-
Notifications
You must be signed in to change notification settings - Fork 861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[timeseries] Fix prediction failing if context contains < 3 observations #4070
[timeseries] Fix prediction failing if context contains < 3 observations #4070
Conversation
|
2 similar comments
|
|
f309903
to
a3d0bc4
Compare
|
Job PR-4070-a3d0bc4 is done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks! Just one question.
@@ -234,13 +234,6 @@ def _get_hpo_backend(self): | |||
|
|||
def _deferred_init_params_aux(self, dataset: TimeSeriesDataFrame) -> None: | |||
"""Update GluonTS specific parameters with information available only at training time.""" | |||
self.freq = dataset.freq or self.freq | |||
if not self.freq: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1/ Why remove self.freq = dataset.freq or self.freq
? We never depend on deferred initialization for freq in any other model? Is freq always set for all models by the trainer?
2/ Why remove this assertion? maybe assert self.freq
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- No other model currently depends on deferred initialization of
freq
(both local & MLForecast models assume thatfreq
is provided at creation time); trainer always setsfreq
when creating models (https://github.com/autogluon/autogluon/blob/master/timeseries/src/autogluon/timeseries/trainer/abstract_trainer.py#L576) - Assuming you are referring to the assertion inside
SimpleGluonTSDataset.__init__
: after pandas 2.2 upgrade, we will anyway need to remove this part completely and always feed a dummy frequencyW
to GluonTS. So the assertion would be removed anyway after [WIP] Upgrade pandas to 2.2 #4074 is merged.
Description of changes:
data.freq
of the data passed tomodel.predict(data)
when generating the forecast. It may happen that some time series indata
passed topredict
contain just a single observation. In this case,freq
inference is impossible and the model fails.self.freq
attribute of the model instead ofdata.freq
whenever frequency is requireddata.fill_missing_values()
works even nofreq
is available (as this is too strong of a requirement for this method)By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.