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

Feat/local and global models in EnsembleModel #1745

Merged
merged 24 commits into from May 26, 2023

Conversation

madtoinou
Copy link
Collaborator

@madtoinou madtoinou commented May 5, 2023

Fixes #1492, fixes #737, fixes #1038.

Summary

EnsembleModel.models now accept a mix of Local and GlobalForecastingModel. If the ensemble contains at least one local model, it only supports single TimeSeries training/inference.

If past/future covariates are provided, they will be passed only the models supporting them (not enforcing same covariates for all the models). I don't know if this is desirable or if all the models should support the exact same covariates in order to belong to the same EnsembleModel?

Other Information

I wonder if we should also allow pre-trained GlobalForecastingModel in EnsembleModel with a flag to bypass the exception currently raised (and make sure the user is aware of the implications). This could be quite powerful but would open the door for data leakage that could be difficult to detect:

  • global model pre-trained with the same target series but too many timestamps : detectable since the ts is stored
  • global model pre-trained with multiple series, including the target series (to eventually bypass the limitation of the training with a single ts in the RegressionEnsembleModel) : undetectable

…single ts training/inference only. If provided, the covariates are passed only to the models supporting them (individually)
@madtoinou madtoinou added this to In review in darts via automation May 5, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 15, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.12 ⚠️

Comparison is base (52f4004) 94.23% compared to head (6a0970d) 94.11%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1745      +/-   ##
==========================================
- Coverage   94.23%   94.11%   -0.12%     
==========================================
  Files         125      125              
  Lines       11581    11593      +12     
==========================================
- Hits        10913    10911       -2     
- Misses        668      682      +14     
Impacted Files Coverage Δ
darts/models/forecasting/theta.py 90.27% <ø> (ø)
darts/models/forecasting/baselines.py 100.00% <100.00%> (ø)
darts/models/forecasting/ensemble_model.py 96.59% <100.00%> (+1.13%) ⬆️
...ts/models/forecasting/regression_ensemble_model.py 100.00% <100.00%> (ø)
darts/models/forecasting/regression_model.py 97.82% <100.00%> (+0.03%) ⬆️

... and 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@dennisbader dennisbader left a comment

Choose a reason for hiding this comment

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

Also a very nice addition, thanks :) 🚀

Just had some minor comments.

Regarding the pre-trained models, I'm with you on this one and think we shouldn't go with this for now.

darts/models/forecasting/ensemble_model.py Outdated Show resolved Hide resolved
darts/models/forecasting/ensemble_model.py Show resolved Hide resolved
darts/models/forecasting/ensemble_model.py Outdated Show resolved Hide resolved
darts/models/forecasting/theta.py Show resolved Hide resolved
madtoinou and others added 3 commits May 22, 2023 13:26
Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>
… added tests for covariates support in ensemble models
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Collaborator

@dennisbader dennisbader left a comment

Choose a reason for hiding this comment

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

Nice, we're almost there 🚀
Mainly minor suggestions, and a bug I found using regression models with different covariates support.

darts/models/forecasting/ensemble_model.py Outdated Show resolved Hide resolved
darts/models/forecasting/ensemble_model.py Outdated Show resolved Hide resolved
darts/models/forecasting/ensemble_model.py Outdated Show resolved Hide resolved
darts/models/forecasting/ensemble_model.py Outdated Show resolved Hide resolved
darts/models/forecasting/ensemble_model.py Outdated Show resolved Hide resolved
darts/tests/models/forecasting/test_ensemble_models.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@dennisbader dennisbader left a comment

Choose a reason for hiding this comment

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

Everything looks great now! 💯 Thanks a lot @madtoinou 🚀

Copy link
Collaborator

@dennisbader dennisbader left a comment

Choose a reason for hiding this comment

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

Everything looks great now! 💯 Thanks a lot @madtoinou 🚀

@dennisbader dennisbader merged commit ee53a83 into master May 26, 2023
9 checks passed
darts automation moved this from In review to Done May 26, 2023
@dennisbader dennisbader deleted the feat/local-and-global-ensemble branch May 26, 2023 14:12
alexcolpitts96 pushed a commit to alexcolpitts96/darts that referenced this pull request May 31, 2023
* feat: remove restriction in EnsembleModel, models can be a mix of Local and Global models

* feat: EnsembleModel accepts a mixture of local and global models for single ts training/inference only. If provided, the covariates are passed only to the models supporting them (individually)

* feat: updated unittests

* doc: fix typo in docstring, SeasonalityMode must be imported from darts.utils.utils

* doc: updated changelog

* feat: logger info when all the models in the ensemble do not support the same covariates, to make the behavior more transparent

* fix: typo, using parenthesis to call proterty method

* Apply suggestions from code review

Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>

* fix: made the covariates handling in ensemble model more transparent, added tests for covariates support in ensemble models

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update CHANGELOG.md

* Apply suggestions from code review

Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>

* fix: addressed reviewer comments, added show_warning arg to ensemble_model, support_*_covariates for RegressionModels rely on the lags attribute

* fix typo

* fix: improve warning synthax

---------

Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>
@dennisbader dennisbader moved this from Done to Released in darts Aug 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
darts
Released
3 participants