Add MADTransform #441

julia-shenshina · 2022-01-10T14:42:41Z

IMPORTANT: Please do not create a Pull Request without creating an issue first.

Before submitting (must do checklist)

Did you read the contribution guide?
Did you update the docs? We use Numpy format for all the methods and classes.
Did you write any new necessary tests?
Did you update the CHANGELOG?

Type of Change

Examples / docs / tutorials / contributors update
Bug fix (non-breaking change which fixes an issue)
Improvement (non-breaking change which improves an existing feature)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Proposed Changes

Related Issue

Closing issues

closes #426

alex-hse-repository · 2022-01-11T06:31:38Z

etna/transforms/math/statistics.py

+            name of processed column
+        window: int
+            size of window to aggregate
+        out_column: str, optional


Many it would be better to put it in the last place

fixed it in all the statistics Transforms 👌🏼

alex-hse-repository · 2022-01-11T06:35:34Z

etna/transforms/math/statistics.py

+        self.min_periods = min_periods
+        self.fillna = fillna
+        self.out_column = out_column
+        super().__init__(


Better to follow the order of the parameters of the superclass

Arguments orders in WindowStatisticsTransform and all it's inheritors are different.
I think it's better to make MADTransform's argument order the same with MeanTransform, MaxTransform, etc.
Moreover, I think that we should change the arguments order in WindowStatisticsTransform but not at this PR (looks like big change).

alex-hse-repository · 2022-01-11T06:39:35Z

tests/test_transforms/test_math/test_statistics_transform.py

+    df["target"] = [-1, 1, 3, 2, 4, 9, 8, 5, 6, 0]
+    df["segment"] = "segment_1"
+
+    df = df.pivot(index="timestamp", columns="segment")


TSDataset.to_dataset ?

fixed all the fixtures here 👌🏼

alex-hse-repository · 2022-01-11T06:45:08Z

tests/test_transforms/test_math/test_statistics_transform.py

@@ -177,6 +210,7 @@ def test_std_feature(simple_df_for_agg: pd.DataFrame, window: int, periods: int,
        MeanTransform(in_column="target", window=5),
        StdTransform(in_column="target", window=5),
        StdTransform(in_column="target", window=5),
+        MADTransform(in_column="target", window=5),
    ),
 )
 def test_fit_transform_with_nans(transform, ts_diff_endings):


This is a method was created to check that transforms simply don't fail working with NaNs. May you also create test that checks that this transform handles NaNs correctly.

Mr-Geekman · 2022-01-11T07:01:58Z

tests/test_transforms/test_math/test_statistics_transform.py

@@ -39,6 +54,8 @@ def simple_df_for_agg() -> pd.DataFrame:
        (MeanTransform, "test_mean"),
        (StdTransform, None),
        (StdTransform, "test_std"),
+        (MADTransform, None),
+        (MADTransform, "test_mad"),
    ),
 )
 def test_interface_simple(simple_df_for_agg: pd.DataFrame, class_name: Any, out_column: str):


May be it is better to add docstring with description to the test like "Test that everything is fine"(oversimplified).

@iKintosh says no comments for tests 😄

codecov-commenter · 2022-01-12T13:34:29Z

Codecov Report

Merging #441 (686c37c) into master (5ef9223) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
+ Coverage   87.64%   87.68%   +0.03%     
==========================================
  Files         112      112              
  Lines        5276     5293      +17     
==========================================
+ Hits         4624     4641      +17     
  Misses        652      652

Impacted Files	Coverage Δ
etna/transforms/__init__.py	`100.00% <100.00%> (ø)`
etna/transforms/math/__init__.py	`100.00% <100.00%> (ø)`
etna/transforms/math/statistics.py	`98.54% <100.00%> (+0.17%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ef9223...686c37c. Read the comment docs.

alex-hse-repository · 2022-01-14T06:47:26Z

tests/test_transforms/test_math/test_statistics_transform.py

@@ -177,6 +227,7 @@ def test_std_feature(simple_df_for_agg: pd.DataFrame, window: int, periods: int,
        MeanTransform(in_column="target", window=5),
        StdTransform(in_column="target", window=5),
        StdTransform(in_column="target", window=5),


May you also remove duplicates from the list of parameters here:pray:

julia-shenshina added 2 commits January 10, 2022 17:40

Add MADTransform

f76a4a8

Upd CHANGELOG

215ff07

julia-shenshina requested a review from alex-hse-repository January 10, 2022 14:43

Add interface test

b591ccd

julia-shenshina self-assigned this Jan 10, 2022

alex-hse-repository suggested changes Jan 11, 2022

View reviewed changes

Mr-Geekman reviewed Jan 11, 2022

View reviewed changes

julia-shenshina added 2 commits January 12, 2022 12:20

Fix docs in statistics transforms

443cbe7

Add test for MADTransform x NaN

686c37c

julia-shenshina requested review from alex-hse-repository and Mr-Geekman January 12, 2022 13:30

alex-hse-repository suggested changes Jan 14, 2022

View reviewed changes

julia-shenshina added 2 commits January 14, 2022 11:01

Delete duplicated tests

6dfc218

Resolve conflicts

b96f57e

alex-hse-repository self-requested a review January 14, 2022 08:15

alex-hse-repository approved these changes Jan 14, 2022

View reviewed changes

julia-shenshina merged commit 445fe85 into master Jan 14, 2022

martins0n deleted the issue426 branch January 21, 2022 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MADTransform #441

Add MADTransform #441

julia-shenshina commented Jan 10, 2022 •

edited

Loading

alex-hse-repository Jan 11, 2022

julia-shenshina Jan 12, 2022

alex-hse-repository Jan 11, 2022

julia-shenshina Jan 12, 2022

alex-hse-repository Jan 11, 2022

julia-shenshina Jan 12, 2022

alex-hse-repository Jan 11, 2022

julia-shenshina Jan 12, 2022

Mr-Geekman Jan 11, 2022

julia-shenshina Jan 12, 2022

codecov-commenter commented Jan 12, 2022 •

edited

Loading

alex-hse-repository Jan 14, 2022

julia-shenshina Jan 14, 2022

Add MADTransform #441

Add MADTransform #441

Conversation

julia-shenshina commented Jan 10, 2022 • edited Loading

Before submitting (must do checklist)

Type of Change

Proposed Changes

Related Issue

Closing issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 12, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julia-shenshina commented Jan 10, 2022 •

edited

Loading

codecov-commenter commented Jan 12, 2022 •

edited

Loading