Skip to content

Update RateOfChange input types to only accept Datetime time index#2408

Merged
thehomebrewnerd merged 3 commits intomainfrom
issue2395-rate-of-change-bug
Dec 16, 2022
Merged

Update RateOfChange input types to only accept Datetime time index#2408
thehomebrewnerd merged 3 commits intomainfrom
issue2395-rate-of-change-bug

Conversation

@thehomebrewnerd
Copy link
Copy Markdown
Contributor

Update RateOfChange input types to only accept Datetime time index

Fixes #2395

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 16, 2022

Codecov Report

Merging #2408 (1afe902) into main (3ed040d) will increase coverage by 29.55%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main    #2408       +/-   ##
===========================================
+ Coverage   69.89%   99.44%   +29.55%     
===========================================
  Files         326      326               
  Lines       20208    20208               
===========================================
+ Hits        14124    20096     +5972     
+ Misses       6084      112     -5972     
Impacted Files Coverage Δ
...tives/standard/transform/numeric/rate_of_change.py 100.00% <100.00%> (ø)
featuretools/entityset/entityset.py 99.22% <0.00%> (+0.12%) ⬆️
...s/computational_backends/feature_set_calculator.py 98.69% <0.00%> (+0.78%) ⬆️
featuretools/computational_backends/feature_set.py 100.00% <0.00%> (+0.93%) ⬆️
featuretools/synthesis/deep_feature_synthesis.py 99.77% <0.00%> (+1.10%) ⬆️
featuretools/synthesis/utils.py 100.00% <0.00%> (+2.77%) ⬆️
...tandard/transform/natural_language/count_string.py 100.00% <0.00%> (+2.85%) ⬆️
...ansform/natural_language/number_of_unique_words.py 100.00% <0.00%> (+3.03%) ⬆️
featuretools/utils/time_utils.py 95.16% <0.00%> (+3.22%) ⬆️
...s/standard/transform/latlong/cityblock_distance.py 100.00% <0.00%> (+3.70%) ⬆️
... and 89 more

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

Copy link
Copy Markdown
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.

Do we need to add a test that numeric time indexes don't work?

@thehomebrewnerd
Copy link
Copy Markdown
Contributor Author

Do we need to add a test that numeric time indexes don't work?

I think that should already be covered by our features tests - we know DFS doesn't build features with input types that are not specified, so adding a specific test here seems unnecessary. The inclusion of all time index types in the primitive definition before was just an oversight.

@thehomebrewnerd thehomebrewnerd enabled auto-merge (squash) December 16, 2022 21:37
@sbadithe
Copy link
Copy Markdown
Contributor

Do we need to add a test that numeric time indexes don't work?

I think that should already be covered by our features tests - we know DFS doesn't build features with input types that are not specified, so adding a specific test here seems unnecessary. The inclusion of all time index types in the primitive definition before was just an oversight.

Makes sense to me!

@thehomebrewnerd thehomebrewnerd merged commit e01f373 into main Dec 16, 2022
@thehomebrewnerd thehomebrewnerd deleted the issue2395-rate-of-change-bug branch December 16, 2022 21:53
@thehomebrewnerd thehomebrewnerd mentioned this pull request Jan 5, 2023
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.

RateOfChange primitive does not work with a numeric time index

3 participants