-
Notifications
You must be signed in to change notification settings - Fork 907
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/scalar with window #2529
Feat/scalar with window #2529
Conversation
…th-window # Conflicts: # darts/models/forecasting/forecasting_model.py # darts/models/forecasting/regression_model.py # darts/utils/historical_forecasts/optimized_historical_forecasts_regression.py # darts/utils/historical_forecasts/utils.py
Co-authored-by: Dennis Bader <[email protected]>
…nsform without model retrain
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2529 +/- ##
==========================================
- Coverage 94.15% 94.10% -0.05%
==========================================
Files 139 139
Lines 14890 14992 +102
==========================================
+ Hits 14019 14108 +89
- Misses 871 884 +13 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
View / edit / reply to this conversation on ReviewNB dennisbader commented on 2024-11-12T09:30:45Z Line #8. "enable_optimization": False, maybe remove? madtoinou commented on 2024-11-12T10:51:33Z I added it to hide the warnings, but I don't mind removing it. |
View / edit / reply to this conversation on ReviewNB dennisbader commented on 2024-11-12T09:30:46Z Here I think it would be great to leave the example without
madtoinou commented on 2024-11-12T11:13:35Z Updated the notebook; two examples (one with, one without the auto-scaling) while minimizing the changes |
I added it to hide the warnings, but I don't mind removing it. View entire conversation on ReviewNB |
…ording to revire comments
Updated the notebook; two examples (one with, one without the auto-scaling) while minimizing the changes View entire conversation on ReviewNB |
View / edit / reply to this conversation on ReviewNB dennisbader commented on 2024-11-14T14:59:43Z I think the note about parallelization was removed, let's add it back again? |
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 looks really good @madtoinou 🚀
I added a couple of suggestions, and two things which we should add:
- support data transformers in residuals
- support pre-trained data transformers regardless of
global_fit
darts/tests/utils/historical_forecasts/test_historical_forecasts.py
Outdated
Show resolved
Hide resolved
darts/tests/utils/historical_forecasts/test_historical_forecasts.py
Outdated
Show resolved
Hide resolved
darts/tests/utils/historical_forecasts/test_historical_forecasts.py
Outdated
Show resolved
Hide resolved
darts/tests/utils/historical_forecasts/test_historical_forecasts.py
Outdated
Show resolved
Hide resolved
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.
Beautiful 😍 I pushed some minor changes:
- adapt the docs a bit here and there
- renamed
idx_series
toseries_idx
- converted
transform()
of fitted_values (in gridsearch) toinverse_transform()
(should be inverse transform I think ;) )
There is one last change that we throw a warning instead of an error when global_fit=True
and len(series)>1
darts/tests/utils/historical_forecasts/test_historical_forecasts.py
Outdated
Show resolved
Hide resolved
…ue and data transformer defined with global_fit=True
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.
Great work @madtoinou and @JanFidor 🚀 Thanks for all the work!
Checklist before merging this PR:
Fixes #1540.
Summary
Scaler/Pipeline
tohistorical_forecasts()
,backtest()
andgridsearch()
to avoid data-leakageenable_optimization=True
, thePipeline
must already be fitted (when applicable), all the series are transformed in one passPipeline
are systematically fitted and applied to the series between each forecast horizon (regardless of theretrain
parameter value)Other Information
The input series to these methods must always be "un-processed" when providing
data_transformers
in order to avoid "double scaling" of the series.This PR is based on #2021.