Add ability to use rolling primitives with pandas' offset aliases #1809

tamargrey · 2021-12-14T21:31:46Z

Allows users to pass strings like 1D or 1H as gap and window_length parameters to the rolling transform primitives. This allows data that does not have a uniform sampling frequency to be used accurately with the rolling transform primitives.

closes #1757

To Do:

codecov · 2021-12-15T23:06:48Z

Codecov Report

Merging #1809 (81eb9af) into main (35f65d7) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1809      +/-   ##
==========================================
+ Coverage   98.80%   98.82%   +0.02%     
==========================================
  Files         144      145       +1     
  Lines       15984    16268     +284     
==========================================
+ Hits        15793    16077     +284     
  Misses        191      191

Impacted Files	Coverage Δ
...primitives/standard/rolling_transform_primitive.py	`100.00% <100.00%> (ø)`
featuretools/primitives/utils.py	`99.51% <100.00%> (+0.12%)`	⬆️
...ools/tests/primitive_tests/test_primitive_utils.py	`100.00% <100.00%> (ø)`
...ls/tests/primitive_tests/test_rolling_primitive.py	`100.00% <100.00%> (ø)`
featuretools/tests/primitive_tests/utils.py	`100.00% <100.00%> (ø)`
...ols/tests/synthesis/test_deep_feature_synthesis.py	`99.33% <100.00%> (+<0.01%)`	⬆️

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 35f65d7...81eb9af. Read the comment docs.

featuretools/primitives/standard/rolling_transform_primitive.py

thehomebrewnerd

There is a lot to digest here, but here are a couple things I've noticed so far.

thehomebrewnerd · 2021-12-17T18:05:55Z

featuretools/tests/primitive_tests/utils.py

@@ -0,0 +1,5 @@
+def get_number_of_days(offset):


Probably not a big deal since we are only using this in tests, but I think this would return the wrong value if we ever used this in a test with a number above 9.

Yeah, I can either clarify the limitations in a docstring/what the intended use is or update it to return any numeric characters before a string. I think my preference for now would be to keep it simple and add a docstring.

Still need to add the docstring here?

oops, yeah forgot to push

thehomebrewnerd · 2021-12-17T18:18:28Z

featuretools/primitives/standard/rolling_transform_primitive.py

+    def _offset_max(self, series):
+        return _apply_roll_with_offset_gap(series, self.gap, max, self.min_periods)
+


I'm not sure it matters too much, but do we need these _offset* functions as methods on the primitive? I haven't tested this to confirm, but I think you can just call _apply_roll_with_offset_gap directly in the rolled_series.apply call below, passing in the additional required arguments. Something like this:

if isinstance(self.gap, str): additional_args = (self.gap, max, self.min_periods) return rolled_series.apply(_apply_roll_with_offset_gap, args=additional_args)

Good call! Updated to do this

tamargrey · 2021-12-21T16:13:17Z

featuretools/primitives/standard/rolling_transform_primitive.py

@@ -251,13 +386,20 @@ def __init__(self, window_length=3, gap=0, min_periods=1):
        self.gap = gap
        self.min_periods = min_periods

+    def _pandas_std(self, series):
+        return series.std()


Tried using np.std, but it seems like there's a slightly different handling of nans that produces different results from the pandas implementation

If you want, you can define this as a function inside the scope of rolling_std since that is the only place it's used, rather than adding it as a class method.

that makes sense! changing

thehomebrewnerd

A couple more minor things - I'd still like to go through this again to solidify my understanding of what is happening exactly, but nothing major is jumping out to me so far.

thehomebrewnerd · 2021-12-22T14:11:05Z

featuretools/primitives/standard/rolling_transform_primitive.py

@@ -251,13 +386,20 @@ def __init__(self, window_length=3, gap=0, min_periods=1):
        self.gap = gap
        self.min_periods = min_periods

+    def _pandas_std(self, series):
+        return series.std()


If you want, you can define this as a function inside the scope of rolling_std since that is the only place it's used, rather than adding it as a class method.

thehomebrewnerd · 2021-12-22T14:44:08Z

featuretools/tests/primitive_tests/test_rolling_primitive.py

@@ -109,14 +192,13 @@ def test_rolling_count(rolling_series_pd):
    [
        (0, 2),  # 0 and 1 get treated the same
        (1, 2),
+        (1, 2),


Accidental duplication?

yep, removed!

thehomebrewnerd · 2021-12-22T14:45:42Z

featuretools/tests/primitive_tests/utils.py

@@ -0,0 +1,5 @@
+def get_number_of_days(offset):


Still need to add the docstring here?

thehomebrewnerd · 2021-12-27T15:56:46Z

featuretools/tests/primitive_tests/test_rolling_primitive.py

+    else:
+        num_nans = gap_num + num_nans_from_min_periods - 1
+
+    # The extra 1 at the beinning is because the std pandas function returns NaN if there's only one value


beinning -> beginning

davesque

I looked over the recent commit that includes the value checking stuff I suggested. That part looks good! The rest is quite a lot so I'll have to spend some more time on it.

freddyaboulton · 2022-01-10T22:41:53Z

featuretools/primitives/utils.py

+        # Add the window_size and gap so that the rolling operation correctly takes gap into account.
+        # That way, we can later remove the gap rows in order to apply the primitive function
+        # to the correct window
+        functional_window_length = to_offset(window_size) + to_offset(gap)


This implementation makes sense to me

freddyaboulton

@tamargrey Thank you for the work on this! I agree with the implementation you chose where we compute window_end - gap to account for the gap. I left a comment on adding coverage but what you have here (and some local tests in a jupyter notebook) matches what I'd expect.

freddyaboulton · 2022-01-10T22:46:34Z

featuretools/tests/primitive_tests/test_rolling_primitive.py

+
+
+@pytest.mark.parametrize("primitive", [RollingCount, RollingMax, RollingMin, RollingMean])
+def test_rolling_primitives_non_uniform_data(primitive):


Might be worth it to actually pre-compute the expected series and check with assert_series_equal? My reasoning is that in two weeks when we ask ourselves how this works, it'll be easier/faster to just look at the expected answer in the unit tests.

That being said, I stepped through this manually and the resulting series matches what I'd expect. Right now, the window size in this test only includes one observation so all the primitives return the same result. I think it'd be good if we test on some situations where there is more than one observation per window.

Maybe something like this could work:

import pandas as pd from featuretools.primitives import RollingMean, RollingCount # When the data isn't uniform, this impacts the number of values in each rolling window datetimes = (list(pd.date_range(start='2017-01-01', freq='1d', periods=3)) + list(pd.date_range(start='2017-01-10', freq='2d', periods=4)) + list(pd.date_range(start='2017-01-22', freq='1d', periods=7))) no_freq_series = pd.Series(range(len(datetimes)), index=datetimes)

@freddyaboulton both good ideas! I added two tests in 4f90dd2 that test the primitives on a series that has windows with multiple observations present and use assert_series_equal. If the tests from this commit were along the lines of what you were thinking, I'll go ahead and update the other tests.

Also updated a test in test_primitive_utils to use the same approach for non uniform series: b0e5e25

rwedge · 2022-01-20T20:15:52Z

featuretools/tests/primitive_tests/test_primitive_utils.py

+@pytest.mark.parametrize(
+    "window_length",
+    [
+        3,
+        "3d"
+    ]
+)


just a style thing but this seems suitable to collapse to one or a a couple lines, given it's only 2 settings for a single parameter

collapsed to one line!

rwedge · 2022-01-20T21:35:37Z

test-requirements.txt

@@ -11,3 +11,4 @@ boto3>=1.17.46
 composeml>=0.3.0
 urllib3>=1.26.5
 pyarrow>=3.0.0
+mock==4.0.3


Can we use the builtin version of mock instead?

https://docs.python.org/dev/library/unittest.mock.html#unittest.mock.patch

rwedge

LGTM!

gsheni assigned tamargrey Dec 14, 2021

tamargrey marked this pull request as ready for review December 16, 2021 21:14

tamargrey requested review from thehomebrewnerd, freddyaboulton and rwedge December 16, 2021 21:15

rwedge reviewed Dec 16, 2021

View reviewed changes

featuretools/primitives/standard/rolling_transform_primitive.py Outdated Show resolved Hide resolved

thehomebrewnerd reviewed Dec 17, 2021

View reviewed changes

tamargrey commented Dec 21, 2021

View reviewed changes

tamargrey requested review from thehomebrewnerd and rwedge December 21, 2021 19:35

thehomebrewnerd reviewed Dec 22, 2021

View reviewed changes

tamargrey force-pushed the rolling-offset branch from cf12c7d to ce7348d Compare December 22, 2021 16:01

thehomebrewnerd previously approved these changes Dec 27, 2021

View reviewed changes

Tamar Grey added 16 commits December 27, 2021 11:49

Add ability to use offset strings in rolling gap util

981c781

handle getting of numer from offset better

d51f55c

Handle non fixed frequencies

92a3b1f

Add additional test for window length when non uniform

4eb712d

Update primitive tests broken for count

3768565

Get offset string gap working for count

3535ad0

Better parameterize rolling count tests

3ebb0f2

Add function to handle offset gap

4c0ab20

Stop handling offset gap in main rolling function

f2b47e1

get apply offset function working in rolling max primitive

78e6337

clean up apply roll primitive logic

447c9b0

update primitive util tests

bfa1c4f

Update primitives

8455133

Add non uniform primitive tests

0069d38

lint fix

f569d1d

fix logic for determining gap rows

80c70fe

Tamar Grey added 7 commits December 27, 2021 11:49

lint fix

8891c34

clean up utils

bca8d8b

clean up tests

198e4d0

fix typo

ff68535

Update arg passing into apply

0dba52c

Add docstring and rename ofsset string util

98afca3

PR comments

2a2be98

tamargrey dismissed thehomebrewnerd’s stale review via 2a2be98 December 27, 2021 20:12

tamargrey force-pushed the rolling-offset branch from ce7348d to 2a2be98 Compare December 27, 2021 20:12

tamargrey mentioned this pull request Jan 3, 2022

[DO NOT MERGE] Separate offset gap logic from numeric gap object #1816

Closed

Tamar Grey added 3 commits January 3, 2022 13:49

Merge branch 'main' into rolling-offset

ad09fe9

Merge branch 'main' into rolling-offset

7490b5f

Expand input checking

ca4eb46

davesque reviewed Jan 10, 2022

View reviewed changes

freddyaboulton reviewed Jan 10, 2022

View reviewed changes

Tamar Grey added 3 commits January 11, 2022 14:49

Merge branch 'main' into rolling-offset

620f887

test non uniform with multiple instances in a window

4f90dd2

clean up non uniform tests

b0e5e25

tamargrey requested a review from freddyaboulton January 12, 2022 16:05

rwedge reviewed Jan 20, 2022

View reviewed changes

gsheni requested a review from rwedge January 20, 2022 21:37

Tamar Grey added 4 commits January 21, 2022 15:01

Merge branch 'main' into rolling-offset

8838f82

MR comments

1361ad0

lint fix

b1ec1c7

Merge branch 'main' into rolling-offset

81eb9af

rwedge approved these changes Jan 25, 2022

View reviewed changes

tamargrey merged commit dfdd2b4 into main Jan 25, 2022

tamargrey deleted the rolling-offset branch January 25, 2022 21:58

dvreed77 mentioned this pull request Feb 11, 2022

v1.5.0 #1901

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to use rolling primitives with pandas' offset aliases #1809

Add ability to use rolling primitives with pandas' offset aliases #1809

tamargrey commented Dec 14, 2021 •

edited

Loading

codecov bot commented Dec 15, 2021 •

edited

Loading

thehomebrewnerd left a comment

thehomebrewnerd Dec 17, 2021

tamargrey Dec 21, 2021

thehomebrewnerd Dec 22, 2021

tamargrey Dec 22, 2021

thehomebrewnerd Dec 17, 2021

tamargrey Dec 21, 2021

tamargrey Dec 21, 2021

thehomebrewnerd Dec 22, 2021

tamargrey Dec 22, 2021

thehomebrewnerd left a comment

thehomebrewnerd Dec 22, 2021

thehomebrewnerd Dec 22, 2021

tamargrey Dec 22, 2021

thehomebrewnerd Dec 22, 2021

thehomebrewnerd Dec 27, 2021

davesque left a comment

freddyaboulton Jan 10, 2022

freddyaboulton left a comment

freddyaboulton Jan 10, 2022

tamargrey Jan 11, 2022

tamargrey Jan 12, 2022

rwedge Jan 20, 2022

tamargrey Jan 21, 2022

rwedge Jan 20, 2022

tamargrey Jan 21, 2022

rwedge left a comment

		def _offset_max(self, series):
		return _apply_roll_with_offset_gap(series, self.gap, max, self.min_periods)



		@pytest.mark.parametrize("primitive", [RollingCount, RollingMax, RollingMin, RollingMean])
		def test_rolling_primitives_non_uniform_data(primitive):

Add ability to use rolling primitives with pandas' offset aliases #1809

Add ability to use rolling primitives with pandas' offset aliases #1809

Conversation

tamargrey commented Dec 14, 2021 • edited Loading

codecov bot commented Dec 15, 2021 • edited Loading

Codecov Report

thehomebrewnerd left a 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

Choose a reason for hiding this comment

thehomebrewnerd left a 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

davesque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freddyaboulton left a 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

rwedge left a comment

Choose a reason for hiding this comment

tamargrey commented Dec 14, 2021 •

edited

Loading

codecov bot commented Dec 15, 2021 •

edited

Loading