fix/historical_fore_fut_cov_notarget_lags #1685
Conversation
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 #1685 +/- ##
==========================================
- Coverage 94.15% 94.05% -0.11%
==========================================
Files 125 125
Lines 11384 11371 -13
==========================================
- Hits 10719 10695 -24
- Misses 665 676 +11
... and 8 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.
Thanks @dumjax for fixing this! 🚀
I believe the way we currently handle past covariates doesn't cover all uses cases. I added a comment. Could you have a look?
| end=series.end_time(), | ||
| freq=series.freq, | ||
| ) | ||
| # longest possible time index for target |
There was a problem hiding this comment.
Thanks :) Could you add some unit tests which check the issue?
It's always good when fixing a bug to add a unit test, because we apparently haven't checked it before
|
|
||
| # past covariates longest possible time index | ||
| # longest possible time index for past covariates |
There was a problem hiding this comment.
I'm not sure the past covariates generated indexes cover all use cases:
- What happens with RegressionModel(lags_past_covariates=[-100], output_chunk_length=1)?
The user suppliedpast_covariatesforfit()can actually be much shorter than the targetseries(101 steps shorter, I believe).
With the current logic and the shorterpast_covariates, the intersection would result in a much shorter historic_forecastable_time_index (because of theend=past_covariates.end_time()). We probably need themax_past_cov_lagas well.
I think for future covariates it is correct, so similarly including the max_past_cov lag should probably work - also is the method supposed to also give the predictable indices after the end (when
is_training=False) taking into account the output_chunk_length/max_target_lag? Then we would have to add the max_target_lag to allendparameters
There was a problem hiding this comment.
Thanks @dumjax , looks great! 🚀
Just some minor comments :)
| >>> model = LinearRegressionModel(lags=[3, 5], past_covariates_lags = 4, output_chunk_length=7) | ||
| >>> model.fit(train_series, past_covariates=past_covariates) | ||
| >>> model.extreme_lags | ||
| (-5, 7, -4, None, None) | ||
| (-5, 6, -4, None, None) |
There was a problem hiding this comment.
| (-5, 6, -4, None, None) | |
| (-5, 6, -4, -4, None) |
Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>
Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>
Special case where the lags are just fut covariates
Summary
Other Information