-
Notifications
You must be signed in to change notification settings - Fork 879
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
Last time index fixes #70
Conversation
Codecov Report
@@ Coverage Diff @@
## master #70 +/- ##
=========================================
+ Coverage 87.36% 87.6% +0.24%
=========================================
Files 74 75 +1
Lines 6986 7131 +145
=========================================
+ Hits 6103 6247 +144
- Misses 883 884 +1
Continue to review full report at Codecov.
|
featuretools/entityset/entityset.py
Outdated
if entity.has_time_index(): | ||
entity.set_last_time_index(entity.df[entity.time_index]) | ||
else: | ||
lti = copy.deepcopy(entity.df[entity.time_index]) |
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.
any reason you can't just use entity.df[entity.time_index].copy()
?
featuretools/entityset/entityset.py
Outdated
else: | ||
lti = copy.deepcopy(entity.df[entity.index]) | ||
lti[:] = None | ||
lti = pd.to_datetime(lti) |
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.
did we require lti to be a datetime before? why do we need to convert it here?
Looks good to me! BTW, great job writing tests for this. |
Bugfixes addressing several issues when calculating
last_time_index
:last_time_index
instance values taken from one child entity if that parent instance was not present in another child entitylast_time_index
if the entity had multiple generations of child entities