-
Notifications
You must be signed in to change notification settings - Fork 917
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 already existing forecasts param #1597
add already existing forecasts param #1597
Conversation
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 @JanFidor for this!
Just one comment about the missing historical_forecasts
defnition in the docs.
After that it's ready to be merged!
@@ -907,6 +907,7 @@ def historical_forecasts( | |||
def backtest( | |||
self, | |||
series: Union[TimeSeries, Sequence[TimeSeries]], | |||
historical_forecasts: Union[TimeSeries, Sequence[TimeSeries]] = None, |
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.
Could you add the parameter definition to the docstring?
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.
On it
Hi @dennisbader, I have 3 quick questions about the PR:
|
@dennisbader I added the docstring, so everything should be ready for merging, if you think any of my proposed changes from the previous comment would be useful to the PR just let me know and I'll get on 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.
Hey @JanFidor , thanks again for another nice PR :)
Had only some minor comments.
A small unittest would be nice (maybe to existing backtest tests) to check wether we get identical results with and without provided historical_forecasts
.
Co-authored-by: Dennis Bader <[email protected]>
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #1597 +/- ##
==========================================
- Coverage 94.13% 94.05% -0.08%
==========================================
Files 125 125
Lines 11340 11328 -12
==========================================
- Hits 10675 10655 -20
- Misses 665 673 +8
... and 9 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
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 @JanFidor and thanks for the updates and patience!
Only had minor suggestions mainly for rephrasing the docs.
After that we can merge this one 🚀
Co-authored-by: Dennis Bader <[email protected]>
Co-authored-by: Dennis Bader <[email protected]>
Hi @dennisbader , thanks for the reviews and help with this PR! Special thanks for the suggested changes to the docs, writing them is still not my strong suit and something I'll have to keep working on ; ) |
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.
LGTM, thanks @JanFidor 🚀
Fixes #1283
Summary
For now it's just a rough implementation, once it's green lit by @dennisbader i'll add some tests