-
Notifications
You must be signed in to change notification settings - Fork 895
Automatically generate name for controllable primitives #481
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
Automatically generate name for controllable primitives #481
Conversation
…to generate_controllable_name
Codecov Report
@@ Coverage Diff @@
## master #481 +/- ##
==========================================
+ Coverage 96.23% 96.25% +0.02%
==========================================
Files 104 104
Lines 8658 8714 +56
==========================================
+ Hits 8332 8388 +56
Misses 326 326
Continue to review full report at Codecov.
|
@@ -13,13 +13,16 @@ class AggregationPrimitive(PrimitiveBase): | |||
stack_on_self = True # whether or not it can be in input_types of self | |||
allow_where = True # whether DFS can apply where clause to this primitive | |||
|
|||
def generate_name(self, base_feature_names, child_entity_id, | |||
parent_entity_id, where_str, use_prev_str): | |||
def generate_name(self, base_feature_names, child_entity_id, parent_entity_id, where_str, use_prev_str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's no reformat the function definition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change applied in this commit.
@@ -51,3 +54,28 @@ def get_function(self): | |||
|
|||
def get_filepath(self, filename): | |||
return os.path.join(config.get("primitive_data_folder"), filename) | |||
|
|||
def get_args_string(self): | |||
if not isinstance(self.__init__, types.MethodType): # __init__ must be defined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is it possible for __init__
not to be defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this example, the __init__
is not defined. I included a test case.
class Primitive(PrimitiveBase):
pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, I tested in python 3 and it worked without this, but just tested in python 2 and it doesn't so I see what you're saying
what if we just added this to PrimitiveBase
?
def __init__(self):
pass
I think that might be a cleaner solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change applied in this commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the test case test_args_string_undefined
be deleted?
if not isinstance(self.__init__, types.MethodType): # __init__ must be defined | ||
return '' | ||
|
||
v2 = version_info.major == 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's try to use the featuretools.primitives.base.utils.inspect_function_args
method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of two functions, could we add an optional argument on inspect_function_args
to restrict keywords like time
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, it might be better to split into two functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's try it with two functions and see how it looks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change applied in this commit.
Creating primitives using |
If a primitive argument is stored as a pandas series, then the generated name won't be very readable. Is there a preferred method on working with these types of arguments?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few more comments. Looking good overall
default = is_same_type and arg.default == getattr(self, arg.name) | ||
return is_positional_or_keyword and not default | ||
|
||
string = OrderedDict() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be an OrderedDict
? why not just make it a list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, doesn't need to be. I will make into list.
string[arg.name] = str(getattr(self, arg.name)) | ||
if len(string) == 0: | ||
return '' | ||
string = ', ' + ', '.join(map('='.join, string.items())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we break this into two lines to make it easier to read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change applied in this commit.
for now, let's not handle the series case specifically. whatever it does by default is fine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one comment about a comment. Otherwise, ready to merge.
error = '"{}" must be attribute of {}' | ||
assert hasattr(self, arg.name), error.format(arg.name, self.__class__.__name__) | ||
|
||
# skip if *args or **kwargs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be "skip if not"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The *args
and **kwargs
would not be considered POSITIONAL_OR_KEYWORD
. This comment could work as well.
# skip if not a standard argument (e.g. excluding *args and **kwargs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
…z/featuretools into generate_controllable_name
No description provided.