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

Check base_of_exclude attribute on Primitive #1749

Merged
merged 14 commits into from Oct 29, 2021
Merged

Conversation

jeff-hernandez
Copy link
Contributor

@jeff-hernandez jeff-hernandez commented Oct 18, 2021

Currently, we check the base_of_exclude attribute on Feature classes instead of Primitive classes which raises an error.

Example

from woodwork.column_schema import ColumnSchema
import featuretools as ft
import woodwork as ww


class Abs(ft.primitives.TransformPrimitive):
    input_types = [ColumnSchema(semantic_tags={'numeric'})]
    name = 'abs'

    def get_function(self):
        return abs

class Sum(ft.primitives.AggregationPrimitive):
    input_types = [ColumnSchema(semantic_tags={'numeric'})]
    base_of_exclude = [Abs]
    name = 'sum'

    def get_function(self):
        return sum

es = ft.demo.load_mock_customer(return_entityset=True)

fd = ft.dfs(
    entityset=es,
    features_only=True,
    agg_primitives=[Sum],
    target_dataframe_name='customers',
)
AttributeError: 'AggregationFeature' object has no attribute 'base_of_exclude'

Related to #1739

@codecov
Copy link

codecov bot commented Oct 18, 2021

Codecov Report

Merging #1749 (e2da4e2) into main (c7ee73c) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1749      +/-   ##
==========================================
+ Coverage   98.63%   98.71%   +0.07%     
==========================================
  Files         138      138              
  Lines       15373    15377       +4     
==========================================
+ Hits        15163    15179      +16     
+ Misses        210      198      -12     
Impacted Files Coverage Δ
featuretools/synthesis/deep_feature_synthesis.py 98.23% <100.00%> (+0.44%) ⬆️
...aturetools/tests/primitive_tests/test_agg_feats.py 97.92% <100.00%> (+0.02%) ⬆️
featuretools/tests/conftest.py 100.00% <0.00%> (+0.40%) ⬆️
...utational_backend/test_calculate_feature_matrix.py 99.45% <0.00%> (+0.65%) ⬆️
...computational_backends/calculate_feature_matrix.py 99.42% <0.00%> (+0.85%) ⬆️

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 c7ee73c...e2da4e2. Read the comment docs.

@jeff-hernandez jeff-hernandez requested a review from a team October 19, 2021 16:59
@jeff-hernandez jeff-hernandez marked this pull request as ready for review October 19, 2021 16:59
@gsheni gsheni requested review from davesque, rwedge, tamargrey and thehomebrewnerd and removed request for a team October 28, 2021 20:33
Comment on lines 203 to 206
test_primitive.stack_on = []
child.primitive.base_of = []
child.primitive.base_of_exclude = [test_primitive]
assert not check_stacking(test_primitive(), [child])
Copy link
Contributor

Choose a reason for hiding this comment

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

if stack_on and base_of are both set to []. check_stacking will return False even if test_primitive is not in base_of_exclude

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh okay, interesting. I changed stack_on and base_of to None so they can return True. It kind of seems like whether None or [] the value should be the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think None means no whitelist while [] means an empty whitelist, so nothing allowed

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

@jeff-hernandez jeff-hernandez merged commit 9d9f831 into main Oct 29, 2021
@jeff-hernandez jeff-hernandez deleted the base-of-exclude branch October 29, 2021 22:02
@rwedge rwedge mentioned this pull request Nov 2, 2021
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.

None yet

2 participants