-
Notifications
You must be signed in to change notification settings - Fork 80
Implement forecast decomposition for SMA-based models #1180
Conversation
🚀 Deployed on https://deploy-preview-1180--etna-docs.netlify.app |
Codecov Report
📣 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 @@ Coverage Diff @@
## master #1180 +/- ##
===========================================
+ Coverage 69.13% 87.64% +18.51%
===========================================
Files 177 177
Lines 10380 10394 +14
===========================================
+ Hits 7176 9110 +1934
+ Misses 3204 1284 -1920
... and 66 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
etna/models/seasonal_ma.py
Outdated
ts.df.loc[:, pd.IndexSlice[:, "target"]] = y_pred | ||
|
||
if return_components: | ||
df.loc[-prediction_size:, pd.IndexSlice[:, "target"]] = y_pred |
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.
We still use df.loc[-prediction_size:, pd.IndexSlice[:, "target"]]
, this probably won't work on all pandas versions (we have error in testing under different pandas versions).
The :prediction_size
implies that first index is integer, but it isn't.
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.
Sorry, missed this place, you are right. Fixed it
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.
May be we should make it more clear why in forecast
we add y_pred
into df
. Because autoregression logic lies within def _forecast
and isn't present on level of def forecast
.
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.
Or we should make it more clear what kind of df
we are going to use inside _predict_components
. We require that it contains lags that was used to make a prediction (taking into account auto-regression), it isn't clear.
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.
Add docstring to _predict_components
with description of df
|
||
Notes | ||
----- | ||
This model supports in-sample and out-of-sample prediction decomposition. |
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 model supports in-sample and out-of-sample prediction decomposition.
I'm not sure that it is an implementation detail and should be in Notes.
Prediction components are corresponding target lags with weights of 1/window
May it could be better to write :math:1 / window
Before submitting (must do checklist)
Proposed Changes
Closing issues
closes #1166