Skip to content

Fix base_of_exclude handling #2380

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

Merged
merged 34 commits into from
Dec 6, 2022
Merged

Fix base_of_exclude handling #2380

merged 34 commits into from
Dec 6, 2022

Conversation

sbadithe
Copy link
Contributor

@sbadithe sbadithe commented Nov 23, 2022

  • closes Fix base_of_exclude handling #2323
  • currently, the check_stacking function is only called during the generation of aggregation features. This means that features such as base_of_exclude and stack_on_exclude do not work when creating transform features. This PR addresses that bug. It also does some refactoring by moving the stack_on_exclude attribute and related member data off of AggregationPrimitiveBase and onto PrimitiveBase, seeing as the functionality provided does not need to be unique to AggregationPrimitives.

@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Merging #2380 (b90db9c) into main (4a88d6f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2380   +/-   ##
=======================================
  Coverage   99.49%   99.50%           
=======================================
  Files         322      322           
  Lines       20106    20146   +40     
=======================================
+ Hits        20005    20046   +41     
+ Misses        101      100    -1     
Impacted Files Coverage Δ
...ools/primitives/base/aggregation_primitive_base.py 100.00% <ø> (ø)
featuretools/primitives/base/primitive_base.py 100.00% <100.00%> (ø)
featuretools/synthesis/deep_feature_synthesis.py 99.77% <100.00%> (+<0.01%) ⬆️
featuretools/synthesis/utils.py 100.00% <100.00%> (ø)
featuretools/tests/conftest.py 100.00% <100.00%> (ø)
...aturetools/tests/primitive_tests/test_agg_feats.py 99.72% <100.00%> (+0.19%) ⬆️
...retools/tests/primitive_tests/test_feature_base.py 100.00% <100.00%> (ø)
...ols/tests/synthesis/test_deep_feature_synthesis.py 99.36% <100.00%> (+0.01%) ⬆️
featuretools/tests/testing_utils/__init__.py 100.00% <100.00%> (ø)
featuretools/tests/testing_utils/features.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.

@sbadithe sbadithe changed the title Fix base_of_exclude handling Fix base_of_exclude handling Nov 23, 2022
@sbadithe sbadithe marked this pull request as ready for review November 23, 2022 23:56
@sbadithe sbadithe requested a review from rwedge November 23, 2022 23:56
@gsheni gsheni requested a review from a team November 30, 2022 20:45
if not primitive_stack_on_self and isinstance(f_primitive, primitive_class):
return False

if isinstance(f_primitive, tup_primitive_stack_on_exclude):
return False

# R TODO: handle this
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussing with @rwedge, determined that this todo was already completed

Copy link
Contributor

@rwedge rwedge left a comment

Choose a reason for hiding this comment

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

LGTM

@sbadithe sbadithe merged commit f804d4b into main Dec 6, 2022
@sbadithe sbadithe deleted the ft-269baseofexclude branch December 6, 2022 17:16
@thehomebrewnerd thehomebrewnerd mentioned this pull request Dec 9, 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.

Fix base_of_exclude handling
2 participants