-
Notifications
You must be signed in to change notification settings - Fork 900
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-2633: Use correct time indices when running historical forecasts with output_chunk_shift in regression models #2634
Conversation
…on regression models with 'output_chunk_shift > 0' and 'output_chunk_length == 1'. Extended unit tests to cover this
if ( | ||
stride == 1 | ||
and model.output_chunk_length == 1 | ||
and model.output_chunk_shift == 0 |
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.
This if-check is extended with an additional condition, which fixes the issue.
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 @MattiasDC for finding and fixing this bug! It's greatly appreciated :)
PR also looks great. I made some changes to the tests (the bounds were not correct, but we didn't properly check that before) and updated the changelog.
I'll merge once all tests have passed. Thanks again 🚀
I'm on my phone now, but you have a typo in test comments 'sampels' iso 'samples' |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2634 +/- ##
==========================================
- Coverage 94.24% 94.19% -0.06%
==========================================
Files 141 141
Lines 15463 15466 +3
==========================================
- Hits 14573 14568 -5
- Misses 890 898 +8 ☔ View full report in Codecov by Sentry. |
…on regression models with 'output_chunk_shift > 0' and 'output_chunk_length == 1'. Extended unit tests to cover this
Checklist before merging this PR:
Fixes #2633.
Summary
When creating historical forecasts using the optimized version, use the correct time indices when
output_chunk_shift > 0
andoutput_chunk_length == 1
. Extended the unit tests to cover this caseOther Information