Apply WindowStatisticsTransform to regressors #474
Conversation
|
🚀 Deployed on https://deploy-preview-474--etna-docs.netlify.app |
| @@ -136,6 +141,12 @@ class MeanTransform(WindowStatisticsTransform): | |||
|
|
|||
| .. math:: | |||
| MeanTransform(x_t) = \\sum_{i=1}^{window}{x_{t - i}\\cdot\\alpha^{i - 1}} | |||
|
|
|||
| This transform, applied to non-regressor column, generates non-regressor column. | |||
There was a problem hiding this comment.
I think we should wrap this in a warning block like this:
Warning
-------
For reference, look at ChangePointsTrendTransform.
etna/transforms/math/statistics.py
Outdated
| @@ -59,6 +60,10 @@ def __init__( | |||
| def fit(self, ts: TSDataset) -> "WindowStatisticsTransform": | |||
| """Fit the transform.""" | |||
| self.in_column_regressor = self.in_column in ts.regressors | |||
| if not self.in_column_regressor: | |||
There was a problem hiding this comment.
I'm not sure if we should give this warning.
A fairly valid approach is to apply MeanTransform to target and then apply LagTransform to the result. It will be strange to force someone to change the order if everything is fine.
However, with new warning we could draw users' attention to the issue. Failed tests are also bothering me, some of them could be problematic and probably should be fixed.
- Keep this warning for a while (we will remove it a little bit later, see second step).
- Fix only the tests where we create a pipeline without lag applied to window transform.
- Ask for re-review (after that we probably remove this warning).
There was a problem hiding this comment.
Ok, we could remove this warning and make other suggested changes.
CHANGELOG.md
Outdated
| @@ -24,7 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| ### Changed | |||
| - | |||
| - | |||
| - | |||
| - Update docstings of `WindowStatisticsTransform`'s parents ([#469](https://github.com/etna-team/etna/pull/474)) | |||
There was a problem hiding this comment.
We should give some more clarification: Add docstring warning about handling non-regressors (including target) to children of WindowStatisticsTransform
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #474 +/- ##
=======================================
Coverage 90.60% 90.60%
=======================================
Files 247 247
Lines 16481 16481
=======================================
Hits 14933 14933
Misses 1548 1548 ☔ View full report in Codecov by Sentry. |
Before submitting (must do checklist)
Proposed Changes
Closing issues
closes #473