Skip to content
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

Apply statsmodels-based ARIMA/VARIMA to new TS #1036

Merged
merged 36 commits into from
Aug 8, 2022
Merged

Conversation

piaz97
Copy link
Contributor

@piaz97 piaz97 commented Jun 24, 2022

Fixes #920 .

Summary

ARIMA and VARIMA models wrapping statsmodels implementations now support the prediction new TS (in addition to the one used during training).

Other Information

  • New base class added, extending DualCovariatesForecastingModel, and adding the support for a series parameter in the predict() method. Not satisfied with the new base class name, StatsmodelsDualCovariatesForecastingModel, any better idea would be welcomed.
  • Adapted both ARIMA and VARIMA models.
  • Added tests.

@piaz97 piaz97 changed the title Apply statsmodels based ARIMA/VARIMA to new TS Apply statsmodels-based ARIMA/VARIMA to new TS Jun 24, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2022

Codecov Report

Merging #1036 (934b204) into master (eb18103) will decrease coverage by 0.00%.
The diff coverage is 96.87%.

@@            Coverage Diff             @@
##           master    #1036      +/-   ##
==========================================
- Coverage   93.75%   93.74%   -0.01%     
==========================================
  Files          80       80              
  Lines        8160     8197      +37     
==========================================
+ Hits         7650     7684      +34     
- Misses        510      513       +3     
Impacted Files Coverage Δ
darts/models/forecasting/forecasting_model.py 96.36% <96.15%> (-0.45%) ⬇️
darts/models/forecasting/varima.py 97.14% <96.66%> (-0.82%) ⬇️
darts/models/forecasting/arima.py 95.23% <100.00%> (+0.64%) ⬆️
darts/timeseries.py 92.15% <0.00%> (-0.07%) ⬇️
...arts/models/forecasting/torch_forecasting_model.py 89.54% <0.00%> (-0.05%) ⬇️
darts/models/forecasting/block_rnn_model.py 98.24% <0.00%> (-0.04%) ⬇️
darts/models/forecasting/nhits.py 98.55% <0.00%> (-0.02%) ⬇️
darts/datasets/__init__.py 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@piaz97 piaz97 marked this pull request as ready for review June 27, 2022 09:17
Copy link
Contributor

@brunnedu brunnedu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool addition to darts! 🚀 Left a few minor comments

darts/models/forecasting/forecasting_model.py Outdated Show resolved Hide resolved
darts/models/forecasting/forecasting_model.py Outdated Show resolved Hide resolved
piaz97 and others added 3 commits June 28, 2022 13:03
Co-authored-by: Dustin Brunner <92083143+brunnedu@users.noreply.github.com>
Co-authored-by: Dustin Brunner <92083143+brunnedu@users.noreply.github.com>
Copy link
Contributor

@hrzn hrzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Only a few minor comments and a suggestion name for the new class

darts/models/forecasting/forecasting_model.py Outdated Show resolved Hide resolved
darts/models/forecasting/arima.py Show resolved Hide resolved
darts/models/forecasting/varima.py Outdated Show resolved Hide resolved
Copy link
Contributor

@hrzn hrzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I've identified a few opportunities for (small) performance improvements using copy=False - there might be more that I missed!

Before merging, would you agree to check that we do not unnecessarily prevent historical forecasts with retrain=False anymore? I think all it takes will likely be to override the method _supports_non_retrainable_historical_forecasts() in TransferrableDualCovariatesForecastingModel to return True instead of False.

darts/models/forecasting/arima.py Outdated Show resolved Hide resolved
darts/models/forecasting/arima.py Outdated Show resolved Hide resolved
darts/models/forecasting/arima.py Outdated Show resolved Hide resolved
darts/models/forecasting/varima.py Outdated Show resolved Hide resolved
darts/models/forecasting/varima.py Outdated Show resolved Hide resolved
darts/models/forecasting/varima.py Outdated Show resolved Hide resolved
darts/models/forecasting/varima.py Outdated Show resolved Hide resolved
darts/models/forecasting/varima.py Outdated Show resolved Hide resolved
darts/models/forecasting/varima.py Outdated Show resolved Hide resolved
darts/models/forecasting/varima.py Outdated Show resolved Hide resolved
@hrzn
Copy link
Contributor

hrzn commented Jul 18, 2022

@piaz97 the tests are failing because ignore_time_axes was renamed into ignore_time_axis. Could you rename here too?

@piaz97
Copy link
Contributor Author

piaz97 commented Jul 21, 2022

I added the suggestions from the previous comments, added support for backtesting with retrain=False, and minor fixes. Should be ready to be merged

@hrzn hrzn merged commit 5ceac68 into master Aug 8, 2022
@madtoinou madtoinou deleted the feat/apply-arima-to-new-ts branch July 5, 2023 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apply ARIMA to a new time series
4 participants