-
Notifications
You must be signed in to change notification settings - Fork 904
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
[BUG-FIX] RegressionEnsembleModel : Error Furture covariates/Past Covariates… #1660
[BUG-FIX] RegressionEnsembleModel : Error Furture covariates/Past Covariates… #1660
Conversation
… FutureCovariates | Suggested With Code Fix
added space after comma
…/darts into fix_RegressionEnsemble
@madtoinou Fixed the lint issue. Do let me know if there is anything else I need to check. |
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.
Thank you for contributing to darts, I did a little suggestion to simplify the modification.
Could you please also add a case in the unit test to cover this scenario (EnsembleModel containing LocalForecastingModels and using future covariates during training/prediction)?
elif (past_covariates is not None) and model.supports_past_covariates: | ||
model.fit(series=series, past_covariates=past_covariates) | ||
else: | ||
model.fit(self.training_series) |
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.
if the if self.is_global_ensemble
condition is not verified, it indicates that the EnsembleModel contains only LocalForecastingModels
which do not support past covariates, you can remove this if-else
case.
Also, you could probably also remove the check on past_covariate
since a value of None
will not disrupt the code-flow (as it's the default value in the fit()
method).
A simpler approach would probably be to reuse the approach for the global_ensemble with something like:
if self.is_global_ensemble:
kwargs = dict(series=series)
if model.supports_past_covariates:
kwargs["past_covariates"] = past_covariates
else:
kwargs = dict(series=self.training_series)
if model.supports_future_covariates:
kwargs["future_covariates"] = future_covariates
model.fit(**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.
Noice!! This is much clean... Will update it and write the test too, Thank you.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1660 +/- ##
==========================================
- Coverage 94.11% 94.04% -0.07%
==========================================
Files 125 125
Lines 11447 11452 +5
==========================================
- Hits 10773 10770 -3
- Misses 674 682 +8 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
for global ensemble it will check if supports past covariates. For both local and global model it will future covariates supports
Working on the test case.
|
Tests with local models supports future covariates along with models which don't support. Prophet() is used to represent one which support future covariate
Added blank line before comment
Linted code with black
@Rajesh4AI, you can find a section at the bottom of the contribution guidelines explaining how to install the dev tools and automate the linting |
fixed library import order
@madtoinou Sure, fixed the import sort manually in |
@madtoinou all linting issues have been fixed. Do let me know if i need to do anything else |
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.
Thank you for contributing to darts!
Suggested a minor modification, otherwise the code looks good 🚀
ensemble_models = self.get_local_models() | ||
|
||
# append model which supports future covariates | ||
ensemble_models.append(Prophet()) |
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.
There are discussion to make Prophet optional (imported from another library, installation is not necessarily straight-forward): it would probably to use a simpler model to test this feature such as LinearRegressionModel
or RandomForest
.
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.
Makse sense.. will modify it
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.
Thanks you for adapting the code so promptly!
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.
Looks good, thanks :) I think we can even simplify a bit more :)
@@ -140,17 +140,13 @@ def fit( | |||
self.models = [model.untrained_model() for model in self.models] | |||
|
|||
for model in self.models: | |||
kwargs = dict(series=series) | |||
if self.is_global_ensemble: |
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.
couldn't we just fully ignore the self.is_global_ensemble and do the following for any model:
kwargs = dict(series=series)
if model.supports_past_covariates:
kwargs["past_covariates"] = past_covariates
if model.supports_future_covariates:
kwargs["future_covariates"] = future_covariates
model.fit(**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.
Thanks @Rajesh4AI , looks good 🚀
…ariates… (unit8co#1660) * [BUG-FIX] RegressionEnsembleModel : Error when trying to predict with FutureCovariates | Suggested With Code Fix * linted the code added space after comma * simplified future covariate check for global ensemble it will check if supports past covariates. For both local and global model it will future covariates supports * test RegressionEnsemble Model with future Covariates Tests with local models supports future covariates along with models which don't support. Prophet() is used to represent one which support future covariate * Fixed possible lint issue Added blank line before comment * Lint correction with Black Linted code with black * isort fix manual fixed library import order * Replace Prophet with altenatives * simplify covariate usage --------- Co-authored-by: madtoinou <32447896+madtoinou@users.noreply.github.com> Co-authored-by: dennisbader <dennis.bader@gmx.ch>
… FutureCovariates | Suggested With Code Fix
[BUG-FIX] RegressionEnsembleModel : Error when trying to predict with FutureCovariates | Suggested With Code Fix #1621
Fixes #.
Summary
Other Information
To reproduce the error try this:
from darts.models import RegressionEnsembleModel,AutoARIMA,Prophet
from darts.datasets import AirPassengersDataset
import numpy as np
series_air = AirPassengersDataset().load()
models = [Prophet(),AutoARIMA()]
ensemble_model = RegressionEnsembleModel(forecasting_models=models,regression_train_n_points=12)
Fit would run
ensemble_model.fit(series=series_air[:-36]
,future_covariates=series_air
)
Error: Predict would fail as Fit doesn't fit the models on future covariate due to bug
pred_air=ensemble_model.predict(n=36,series=series_air[:-36],future_covariates=series_air)