-
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
Fix/add fit/predict_kwargs argument to historical_forecasts #2050
Conversation
…elevant for TorchForecastingModels
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.
Wouldn't this be a great opportunity to add fit and predict kwargs to historical forecasts? :)
Codecov ReportAttention: ❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
... and 5 files with indirect coverage changes 📢 Thoughts on this report? Let us know! |
…Models._fit_wrapper
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 like a great start @madtoinou 🚀
I added a couple of suggestions on how we could simplify the kwargs handling and the fit/predict wrappers.
We can let python handle some of edge cases, and potentially, we can adapt the fit/predict wrappers to generalize over all models (and reduce code).
darts/utils/historical_forecasts/optimized_historical_forecasts_torch.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.
Looking really good now, thanks @madtoinou 🚀
Just some minor suggestions
darts/utils/historical_forecasts/optimized_historical_forecasts_torch.py
Show resolved
Hide resolved
…ility to deactivate the warnings
Updated the PR, handling of the |
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.
Very nice, this looks awesome 🚀
This time really only minimal suggestions, we can merge soon.
darts/utils/historical_forecasts/optimized_historical_forecasts_torch.py
Show resolved
Hide resolved
Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>
…lForecastingModel when fitted with a future covariate series
…it/predict throught the wrapper
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.
Everything looks good now, great job @madtoinou 🚀
Can't wait for release |
Fixes #1767.
Summary
fit_kwargs
andpredict_kwargs
argument tohistorical_forecasts
,backtest
andgridsearch
fit_predict
andpredict_wrapper
to pass the arguments tofit()/predict()
only when it's supported (based on method signature)Other Information
Rely on python to complain when unsupported or duplicated arguments are provided