-
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
Add supports_multivariate property to ForecastingModel #1848
Conversation
CC @madtoinou |
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.
Looking nice, thank you for also fixing the type hinting for some method.
In order to slightly simplify the code, would it be possible to define the property as returning True
for the GlobalForecastingModels
while keeping what you did for the LocalForecastingModels
?
PS: it appears that you forgot to redefine the property for the FourTheta
model in "theta.py"
Codecov ReportPatch coverage:
❗ 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 #1848 +/- ##
==========================================
- Coverage 94.16% 93.94% -0.23%
==========================================
Files 125 125
Lines 11642 11724 +82
==========================================
+ Hits 10963 11014 +51
- Misses 679 710 +31
☔ View full report in Codecov by Sentry. |
@madtoinou this should be done now. |
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.
Looking good! Some minors comments before this is ready for merging.
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.
Added suggestions for the docstrings of some RegressionModel
, to make it more explicit that darts wrap around these models to make them multivariate when necessary.
On the contrary, RandomForest
supports multivariate out of the box, same for XGBoost
for example (so no need to mention anything in the docstring).
Co-authored-by: madtoinou <32447896+madtoinou@users.noreply.github.com>
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 really good @felixdivo , thanks a lot 🚀
Only had some minor comments:
- Linting checks fail because of the added docstring for RegressionModels.
- we can let RegressionModel handle the multivariate support and drop the properties in the sub classes
Uniform handling & documentation of supports_multivariate by RegressionModel class Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>
Done :) |
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.
Perfect, thanks a lot @felixdivo 🚀 💯
Fixes #1844.
Summary
The property was added to all models.
Other Information
I just assumed that this property then holds for all acpects of the time series, i.e. targets, covariates, ...