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

Add commutative argument to SubtractNumeric and DivideNumeric primitives #457

Merged
merged 3 commits into from Mar 12, 2019

Conversation

Projects
None yet
2 participants
@grayskripko
Copy link
Contributor

commented Mar 6, 2019

Add commutative argument to SubtractNumeric and DivideNumeric primitives

@codecov

This comment has been minimized.

Copy link

commented Mar 6, 2019

Codecov Report

Merging #457 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #457      +/-   ##
==========================================
+ Coverage   96.46%   96.46%   +<.01%     
==========================================
  Files          98       98              
  Lines        8620     8623       +3     
==========================================
+ Hits         8315     8318       +3     
  Misses        305      305
Impacted Files Coverage Δ
...aturetools/primitives/standard/binary_transform.py 99.65% <100%> (ø) ⬆️

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 afe8c66...6d60e34. Read the comment docs.

@kmax12
Copy link
Member

left a comment

Looking good. Just a few comments

@@ -230,7 +230,9 @@ class SubtractNumeric(TransformPrimitive):
name = "subtract_numeric"

This comment has been minimized.

Copy link
@kmax12

kmax12 Mar 6, 2019

Member

can we add a docstring to this class? Something like this

"""Subtracts one feature from another and returns the difference
    Args:
        commutative (bool): determines if Deep Feature Synthesis should generate
            both a - b and b - a, or just one.  If commutative is False, there is no
            guarantee which of the two will be generated. Defaults to True.
"""

This comment has been minimized.

Copy link
@grayskripko

grayskripko Mar 7, 2019

Author Contributor

@kmax12 it looks strange to add docstring to SubtractNumeric and DivideNumeric classes only. All other primitives don't have docstrings. What do you think?

This comment has been minimized.

Copy link
@kmax12

kmax12 Mar 7, 2019

Member

it's only in binary_transform.py that don't have docstrings. the other primitive files do.

we didn't put them here since most people aren't importing and using the binary primitives directly. however, now that you added a parameter, someone will need to know how to use it.

if you have time, you're more than welcome to add docstrings to the other methods in binary_transform.py!

@@ -308,6 +310,9 @@ class DivideNumeric(TransformPrimitive):
input_types = [Numeric, Numeric]

This comment has been minimized.

Copy link
@kmax12

kmax12 Mar 6, 2019

Member

Please add a docstring for this primitive class too. It can be siimilar to the subtract example I gave

grayskripko and others added some commits Mar 7, 2019

@kmax12 kmax12 self-requested a review Mar 12, 2019

@kmax12

kmax12 approved these changes Mar 12, 2019

Copy link
Member

left a comment

Looks good. Thank you for the contribution!

@kmax12

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

@grayskripko can you sign the CLA so I can merge this in? I think you can do it by clicking here: https://cla-assistant.io/Featuretools/featuretools?pullRequest=457

@grayskripko

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

@grayskripko can you sign the CLA so I can merge this in? I think you can do it by clicking here: https://cla-assistant.io/Featuretools/featuretools?pullRequest=457

I've done it now but, actually, it was the second time

@kmax12

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

You're right. I see you also did sign it on March 6th. Not sure why it didn't update it here. Will go ahead and merge now

@kmax12 kmax12 changed the title Update binary_transform.py Add commutative argument to SubtractNumeric and DivideNumeric primitives Mar 12, 2019

@kmax12 kmax12 merged commit 6e7b919 into Featuretools:master Mar 12, 2019

2 checks passed

codecov/patch 100% of diff hit (target 96.46%)
Details
codecov/project 96.46% (+<.01%) compared to afe8c66
Details

@rwedge rwedge referenced this pull request Mar 29, 2019

Merged

v0.7.0 #477

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.