Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/multivariate wrapper #1917
base: master
Are you sure you want to change the base?
Feature/multivariate wrapper #1917
Changes from 6 commits
83bba23
08999fc
a079c8e
ef4d305
d652e3a
ddc3fd3
c2e7037
ba39d55
48cb156
fb1c06b
450e081
36d3f13
5a0e32a
8c1b573
908cfd4
bf64cff
aea93b1
05af0ca
37226d3
2b4138a
90303d6
908b1ef
f7ca592
7386d9e
219b933
40e87d3
e0eb50b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Instead of the
isinstance
, rather use thesupports_future_covariates
.Have a single call the predict with an inline if-else to pass the future_covariates (check suggestion in the
fit()
).Also, it would be great to at least check that there is the same number of components in the forecasted series as models in the
self_trained_models
attribute.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.
Hi, I tried the approach with if else for passing the covariates, but it was throwing errors for models which didn't support future covariates
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.
Because they were trained without the
future_covariates
(and hence theoretically support them but not in this context, at prediction time?) or because they actually did not support them?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.
Because they actually did not support them, some of the LocalForecastingModels don't support future covariates, so an error is thrown about
future_covariates
not being a parameter for fit() and predict()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.
Your tests are not matching the style of the others tests, please look how they are defined in test_local_forecasting_models.py (especially the parametrize decorator and the helper functions)
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.
Should be fixed now (8c1b573)
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.
Instead of loops in the
test_fit_predict_local_models()
andtest_fit_predict_local_future_covariates_models()
what will make the test fail if one of the model fails, without indicating which one, it would be great to leveragepytest.mark.parametrize
and replace the list of instantiated models with a list of kwargs (one for local, one for local that supports future covariates), that would then be used to create the model before fit/predict (as done here).Also, testing models that support future covariates without actually passing future covariates in
fit()
would be great.