Fix ARIMA not accounting for gap in prediction from end of training data#3884
Conversation
jeremyliweishih
changed the title
Codecov Report
@@ Coverage Diff @@
## main #3884 +/- ##
=======================================
+ Coverage 99.7% 99.7% +0.1%
=======================================
Files 346 346
Lines 36304 36325 +21
=======================================
+ Hits 36167 36188 +21
Misses 137 137
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| from sktime.forecasting.base import ForecastingHorizon | ||
|
|
||
| fh_ = ForecastingHorizon([i + 1 for i in range(len(X))], is_relative=True) | ||
| # we can only calculate the difference if the indices are of the same type |
There was a problem hiding this comment.
set this as a fallback to previous behavior in case we ever receive inconsistent indices
There was a problem hiding this comment.
Is the index type being different indicative of something?
There was a problem hiding this comment.
it should only be the result of inconsistent user behavior and this is a safeguard against that!
jeremyliweishih
requested review from
christopherbunn,
chukarsten,
eccabay and
fjlanasa
eccabay
left a comment
eccabay
left a comment
There was a problem hiding this comment.
Looks pretty good, just had a few questions! Thanks for taking care of this.
| units_diff = len(dates_diff) - 1 | ||
| fh_ = ForecastingHorizon( | ||
| [units_diff + i for i in range(len(X))], |
There was a problem hiding this comment.
Seems like we needed an off-by-one offset in the non-gap case. Should that still be the case here as well?
There was a problem hiding this comment.
We need the + 1 since range(len(X)) starts at 0. In the gap case units_diff should always be > 0 so we don't need it!
| ) | ||
|
|
||
| clf = ARIMARegressor() | ||
| clf.last_X_index = X.index[-1] |
There was a problem hiding this comment.
we never ran fit in this test so last_X_index wasn't being set!
|
|
||
|
|
||
| @pytest.mark.parametrize("use_covariates", [True, False]) | ||
| def test_arima_regressor_can_forecast_arbitrary_dates(use_covariates, ts_data): |
There was a problem hiding this comment.
It might be useful here to highlight how this is arbitrary dates in a comment or something - how long is X_test here, so how big of a gap is it between the training data and what we're asking to predict on?
| assert ( | ||
| arima.predict(X_test).tail(5).tolist() == arima.predict(X_test_last_5).tolist() | ||
| ) |
There was a problem hiding this comment.
Does this fail when run on current main?
chukarsten
left a comment
chukarsten
left a comment
There was a problem hiding this comment.
Just some small things and questions. Little nit to approve.
| from sktime.forecasting.base import ForecastingHorizon | ||
|
|
||
| fh_ = ForecastingHorizon([i + 1 for i in range(len(X))], is_relative=True) | ||
| # we can only calculate the difference if the indices are of the same type |
There was a problem hiding this comment.
Is the index type being different indicative of something?
3 participants
Fixes #3853.