Skip to content

Add MultiplyNumericBoolean primitive that allows for multiplication between a numeric an a boolean column#2035

Merged
thehomebrewnerd merged 14 commits intomainfrom
mult-numeric-bool
Apr 26, 2022
Merged

Add MultiplyNumericBoolean primitive that allows for multiplication between a numeric an a boolean column#2035
thehomebrewnerd merged 14 commits intomainfrom
mult-numeric-bool

Conversation

@thehomebrewnerd
Copy link
Contributor

Add MultiplyNumericBoolean primitive that allows for multiplication between a numeric an a boolean column

Replaces functionality that is removed in #2022 with a more appropriately named primitive.

Closes #2026

@thehomebrewnerd thehomebrewnerd marked this pull request as draft April 26, 2022 16:32
to_test = [
("numeric", "numeric"),
("numeric", "bool"),
("bool", "numeric"),
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this case removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't swap inputs with the new MultiplyBooleanNumeric primitive like we could with the previous MultiplyNumeric primitive. The first input needs to be the numeric column and the second the boolean column.

Or if you prefer, we could add logic in the primitive to determine which is which based on dtype and act accordingly?

Copy link
Contributor

Choose a reason for hiding this comment

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

what if we add logic in the __mul__ override to pass in [numeric, bool] in that order instead of always doing [self, other]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That could work. I'll have a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the primitive logic and inputs to handle this case since updating __mul__ could result in two features with the same name.

@thehomebrewnerd thehomebrewnerd marked this pull request as ready for review April 26, 2022 17:05
rwedge
rwedge previously approved these changes Apr 26, 2022
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.

Looks good, assuming tests pass

@codecov
Copy link

codecov bot commented Apr 26, 2022

Codecov Report

Merging #2035 (29c6332) into main (7028210) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2035      +/-   ##
==========================================
- Coverage   99.09%   99.05%   -0.04%     
==========================================
  Files         143      143              
  Lines       16511    16568      +57     
==========================================
+ Hits        16361    16411      +50     
- Misses        150      157       +7     
Impacted Files Coverage Δ
featuretools/feature_base/feature_base.py 96.89% <100.00%> (+0.01%) ⬆️
...aturetools/primitives/standard/binary_transform.py 100.00% <100.00%> (ø)
...s/tests/primitive_tests/test_transform_features.py 99.86% <100.00%> (+<0.01%) ⬆️
...utational_backend/test_calculate_feature_matrix.py 99.23% <0.00%> (-0.77%) ⬇️

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 7028210...29c6332. Read the comment docs.

es = boolean_mult_es
to_test = [
("numeric", "numeric"),
("numeric", "bool"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwedge These were removed in the PR with changes to MultiplyNumeric, so had to add them back in now.

Copy link
Contributor

@dvreed77 dvreed77 left a comment

Choose a reason for hiding this comment

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

lgtm

@thehomebrewnerd thehomebrewnerd enabled auto-merge (squash) April 26, 2022 18:49
@thehomebrewnerd thehomebrewnerd merged commit bb0e420 into main Apr 26, 2022
@thehomebrewnerd thehomebrewnerd deleted the mult-numeric-bool branch April 26, 2022 19:04
@thehomebrewnerd thehomebrewnerd mentioned this pull request Apr 27, 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.

Need a way to create a seed feature that is the multiplication of a numeric column with a boolean column.

3 participants