[timeseries] Minor bugfixes & improvements for local forecasting models #3252
Conversation
|
Thank you for addressing the issue. I have a question: why doesn't our CI detect the local model's failure and subsequent use of the naive model, given that this occurs consistently? Does this suggest that the naive model's performance is on par with the local model, or is the issue specific to the dataset being used? |
| @@ -71,6 +68,10 @@ def __init__( | |||
| self.n_jobs = n_jobs | |||
| else: | |||
| raise ValueError(f"n_jobs must be a float between 0 and 1 or an integer (received n_jobs = {n_jobs})") | |||
| # Default values, potentially overridden inside _fit() | |||
| self.use_fallback_model = True | |||
| self.max_ts_length = 2500 | |||
There was a problem hiding this comment.
Shall we adjust it based on the freq? For example, for min data, 2500 is less than 2 days which will not be able to capture weekly trends/seasonality
There was a problem hiding this comment.
+1 on at least making it customizable
There was a problem hiding this comment.
@tonyhoo Currently, the models that we use (AutoETS, AutoARIMA, Theta, ETS, ARIMA, Naive, SeasonalNaive) are anyway unable to capture multiple seasonalities - they only capture the seasonality at the seasonal_period that we provide. This is at most 24 * 60 = 1440 for minutely data, but usually much smaller (<= 24).
I think that the example you described would apply to models like MSTL that consider multiple seasonalities, and I agree that we would need to increase the max_ts_length for such models if we add them.
@gradientsky I've moved the parameter override code from the _fit method to __init__.
| Number of CPU cores used to fit the models in parallel. | ||
| When set to a float between 0.0 and 1.0, that fraction of available CPU cores is used. | ||
| When set to a positive integer, that many cores are used. | ||
| When set to -1, all CPU cores are used. | ||
| max_ts_length : int, default = 2500 |
| @@ -71,6 +68,10 @@ def __init__( | |||
| self.n_jobs = n_jobs | |||
| else: | |||
| raise ValueError(f"n_jobs must be a float between 0 and 1 or an integer (received n_jobs = {n_jobs})") | |||
| # Default values, potentially overridden inside _fit() | |||
| self.use_fallback_model = True | |||
| self.max_ts_length = 2500 | |||
There was a problem hiding this comment.
+1 on at least making it customizable
timeseries/src/autogluon/timeseries/models/local/abstract_local_model.py
Outdated
Show resolved
Hide resolved
|
Job PR-3252-05a8937 is done. |
| @@ -3,6 +3,7 @@ | |||
| import pandas as pd | |||
| import pytest | |||
There was a problem hiding this comment.
@tonyhoo Regarding the tests for model performance: Currently, CI for time series does not include any regression tests. The reasoning was the models could be changing quite frequently, and keeping track of the performance ranges for individual models would be tedious. However, I agree that now we should probably be looking to add these tests, as the model set is becoming more stable. Do you think this it's fine if we add these tests after v0.8, or do you think it has higher priority and we should do this asap?
There was a problem hiding this comment.
We can add these post 0.8 and make sure it is incorporated into the benchmark and dashboard project
|
Job PR-3252-156ba1e is done. |
|
Job PR-3252-bc47956 is done. |
|
Job PR-3252-5c3cf7f is done. |
Description of changes:
use_fallback_modelas an optional hyperparameter for all local models (defaultTrue). When set toFalse, fallback model will be disabled, and any exception in the underlying model will propagate. This is important for testing - currently, we had one model that always failed because of a bug, but this wasn't caught by the CI because of the fallback model.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.