-
Notifications
You must be signed in to change notification settings - Fork 871
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
Implementing PartOfDay primitive #2128
Conversation
@@ -0,0 +1,6 @@ | |||
from featuretools.primitives import * |
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.
remove?
Codecov Report
@@ Coverage Diff @@
## main #2128 +/- ##
=======================================
Coverage 99.21% 99.21%
=======================================
Files 143 143
Lines 16869 16907 +38
=======================================
+ Hits 16737 16775 +38
Misses 132 132
Continue to review full report at Codecov.
|
|
||
def get_function(self): | ||
def part_of_day(vals): | ||
return vals.apply(lambda x: self.get_part_of_day(x.hour)) |
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 see if there is a faster way to implement this primitive? Can we vectorize it or look for a faster approach? Perhaps a pd.merge?
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.
It also might be worth doing performance checking and using the fastest approach
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.
Sounds good, I will look into it right now
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.
I'm still trying to get it to work -- haven't been able to find a way that doesn't involve a map of some kind (because the output is not boolean). Does anyone have any tips? Right now it's about 4x slower than something like IsWeekend().
Merge branch 'part-of-day' of https://github.com/alteryx/featuretools into part-of-day
docs/source/release_notes.rst
Outdated
|
||
v1.10.0 June 23, 2022 | ||
===================== | ||
* Enhancements | ||
* Add ``DayOfYear``, ``DaysInMonth``, ``Quarter``, ``IsLeapYear``, ``IsQuarterEnd``, ``IsQuarterStart`` transform primitives (:pr:`2110`, :pr:`2117`) | ||
* Add ``IsMonthEnd``, ``IsMonthStart`` transform primitives (:pr:`2121`) | ||
* Add ``IsMonthEnd``, ``IsMonthStart`` transform primitives (:pr:`2121`, :pr:`2128`) |
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.
Shouldn't there be a new line with PartOfDay and this PR # ?
) | ||
answer = [i if not pd.isna(i) else None for i in answer] | ||
correct_answer = [i if not pd.isna(i) else None for i in correct_answer] | ||
np.testing.assert_array_equal(answer, correct_answer) |
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.
a nit, but convention is to compare expected
and actual
, not answer
and correct_answer
Merge branch 'part-of-day' of https://github.com/alteryx/featuretools into part-of-day
@@ -515,6 +515,53 @@ def month(vals): | |||
return month | |||
|
|||
|
|||
class PartOfDay(TransformPrimitive): |
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.
Sorry, just thought of this, but can you add boundaries of all different types in the doc string?
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.
lgtm
fixes #2059