-
Notifications
You must be signed in to change notification settings - Fork 862
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] Add wrappers for Statsforecast models #2758
Conversation
Job PR-2758-7212948 is done. |
Job PR-2758-a376e62 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.
Are we planning to add them to any popular presets such as medium_quality
or high_quality
?
@@ -32,6 +32,7 @@ | |||
"torch>=1.9,<1.14", | |||
"pytorch-lightning>=1.7.4,<1.9.0", | |||
"networkx", | |||
"statsforecast==1.4.0", |
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.
Can we make 1.4.0
the lower bound?
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.
This way we are protecting ourselves from potentially breaking changes / regressions caused by newer versions of the dependencies. Both things already happened to us a few times (caused by minor releases of sktime & GluonTS), so I would rather be extra cautious here.
""" | ||
# TODO: Find a way to ensure that SF models respect time_limit | ||
# Fitting usually takes >= 20 seconds | ||
if time_limit is not None and time_limit < 20: |
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.
The fit
time should be dependent on instance type as well. If time_limit
is not implemented and we expect the local model fit shall be quick compared with other models, shall we send warning message instead?
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.
Updated this logic based on the discussion below.
# Fitting usually takes >= 20 seconds | ||
if time_limit is not None and time_limit < 20: | ||
raise TimeLimitExceeded | ||
super()._fit(train_data=train_data, time_limit=time_limit, verbosity=verbosity, **kwargs) |
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.
One quick idea to achieve time_limit
is to take advantage of timeout
from ThreadPoolExecutor
or ProcessPoolExecutor
. Details can be found here
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.
I agree but, if I understand correctly, this would require monkey-patching StatsForecast. Is it fine if we leave it as-is for now and add a TODO
comment?
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.
Let's add it in TODO and follow up with that
AutoARIMA=30, | ||
AutoETS=70, | ||
DynamicOptimizedTheta=60, |
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.
How do we determine the priorities of these stats models? How will they added to existing ARIMA and ETA impl? Some accuracy metrics comparison might be helpful to give insights with ensemble results
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.
I set the priorities inversely proportional to the average time it takes to fit these models (slower model -> lower priority).
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.
Also added models to the presets based on the discussion below.
After benchmarking
Based on these findings, I suggest to
I will do another round of benchmarking after adding TFT & updating the SFF model from the new GluonTS release. We can update the presets based on these results. |
Job PR-2758-ed4706a is done. |
Description of changes:
AutoETS
,AutoARIMA
andDynamicOptimizedTheta
models fromStatsForecast
.To Do:
n_jobs
parameterBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.