Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract rolling primitives logic into a general function #2218

Merged
merged 17 commits into from
Aug 8, 2022

Conversation

ozzieD
Copy link
Contributor

@ozzieD ozzieD commented Aug 3, 2022

resolves: #2213

@ozzieD ozzieD changed the title Draft: extract rolling primitives Draft: extract rolling primitives logic into general function Aug 3, 2022
@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #2218 (ca598dd) into main (7f0cbe2) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2218      +/-   ##
==========================================
- Coverage   99.29%   99.29%   -0.01%     
==========================================
  Files         146      146              
  Lines       17594    17583      -11     
==========================================
- Hits        17470    17459      -11     
  Misses        124      124              
Impacted Files Coverage Δ
featuretools/primitives/rolling_primitive_utils.py 100.00% <100.00%> (ø)
...primitives/standard/rolling_transform_primitive.py 100.00% <100.00%> (ø)
...ls/tests/primitive_tests/test_rolling_primitive.py 100.00% <100.00%> (ø)
...ts/primitive_tests/test_rolling_primitive_utils.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ozzieD ozzieD changed the title Draft: extract rolling primitives logic into general function extract rolling primitives logic into general function Aug 3, 2022
@ozzieD ozzieD requested a review from tamargrey August 3, 2022 21:26
[RollingCount, RollingMax, RollingMin, RollingMean, RollingSTD, RollingTrend],
)
@patch("featuretools.primitives.rolling_primitive_utils.apply_roll_with_offset_gap")
def test_no_call_to_apply_roll_with_offset_gap_with_numeric(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test was moved to the utils test since it is testing the utils function more than the actual primitives

window_size="test",
gap="7D",
min_periods=1,
)
Copy link
Contributor Author

@ozzieD ozzieD Aug 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lots of black, pre-commit linting changes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if there were any non linting changes to the actual test checks? I haven't seen any, but just want to make sure I'm not missing anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only change is specifying the min_periods parameter for roll_series_with_gap. I rolled the default parameters up to apply_rolling_agg_to_series so existing functions like roll_series_with_gap no longer need a default value.

Copy link
Contributor

@sbadithe sbadithe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just minor grammar suggestions

featuretools/primitives/rolling_primitive_utils.py Outdated Show resolved Hide resolved
featuretools/primitives/rolling_primitive_utils.py Outdated Show resolved Hide resolved
featuretools/primitives/rolling_primitive_utils.py Outdated Show resolved Hide resolved
featuretools/primitives/rolling_primitive_utils.py Outdated Show resolved Hide resolved
featuretools/primitives/rolling_primitive_utils.py Outdated Show resolved Hide resolved
featuretools/primitives/rolling_primitive_utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@tamargrey tamargrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for doing this! Makes the logic around rolling primitives feel a lot more manageable

featuretools/primitives/rolling_primitive_utils.py Outdated Show resolved Hide resolved
window_size="test",
gap="7D",
min_periods=1,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if there were any non linting changes to the actual test checks? I haven't seen any, but just want to make sure I'm not missing anything.

featuretools/primitives/rolling_primitive_utils.py Outdated Show resolved Hide resolved
featuretools/primitives/rolling_primitive_utils.py Outdated Show resolved Hide resolved
@gsheni gsheni requested a review from sbadithe August 8, 2022 18:31
@gsheni gsheni changed the title extract rolling primitives logic into general function Extract rolling primitives logic into a general function Aug 8, 2022
@gsheni gsheni requested a review from tamargrey August 8, 2022 18:31
@ozzieD ozzieD merged commit 7f25720 into main Aug 8, 2022
@ozzieD ozzieD deleted the 2213-refactor-rolling-prim-logic branch August 8, 2022 18:56
@ozzieD ozzieD mentioned this pull request Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor re-used Rolling primitive logic into separate util function
4 participants