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

Adding relative units into Timedelta #692

Merged
merged 29 commits into from Aug 15, 2019

Conversation

@jeremyliweishih
Copy link
Contributor

commented Aug 1, 2019

Adds functionality for relative units (month/year) and refactored out superfluous functions and fields.

@jeremyliweishih jeremyliweishih changed the title Timedelta Adding relative units into Timedelta Aug 1, 2019

@codecov

This comment has been minimized.

Copy link

commented Aug 1, 2019

Codecov Report

Merging #692 into master will increase coverage by 0.15%.
The diff coverage is 98.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #692      +/-   ##
==========================================
+ Coverage   97.48%   97.63%   +0.15%     
==========================================
  Files         118      118              
  Lines       10214    10166      -48     
==========================================
- Hits         9957     9926      -31     
+ Misses        257      240      -17
Impacted Files Coverage Δ
...mputational_backend/test_feature_set_calculator.py 100% <ø> (ø) ⬆️
featuretools/computational_backends/utils.py 94.64% <100%> (+0.83%) ⬆️
...aturetools/tests/entityset_tests/test_timedelta.py 100% <100%> (ø) ⬆️
...utational_backend/test_calculate_feature_matrix.py 99.35% <100%> (ø) ⬆️
featuretools/utils/wrangle.py 74.71% <100%> (-0.29%) ⬇️
featuretools/entityset/timedelta.py 97.5% <96.77%> (+10.29%) ⬆️

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 97eb274...8168b26. Read the comment docs.

@jeremyliweishih jeremyliweishih requested a review from kmax12 Aug 5, 2019

@kmax12
Copy link
Member

left a comment

looking good. some requested changes

featuretools/entityset/timedelta.py Outdated Show resolved Hide resolved
featuretools/entityset/timedelta.py Outdated Show resolved Hide resolved
featuretools/entityset/timedelta.py Show resolved Hide resolved
unit = freq.unit

freq = str(freq.value) + unit
return dt.apply(lambda x: x.floor(freq))

This comment has been minimized.

Copy link
@kmax12

kmax12 Aug 6, 2019

Member

we should confirm this gives the same results for approximate as before on full integration test suite.

@@ -2,7 +2,7 @@

from featuretools.version import __version__ as ft_version

SCHEMA_VERSION = "3.0.0"
SCHEMA_VERSION = "3.0.1"

This comment has been minimized.

Copy link
@kmax12

kmax12 Aug 6, 2019

Member

let's make this 3.1.0 because it does change the API

@kmax12
Copy link
Member

left a comment

one test case request, otherwise looks good to me.

]

for td, expected in zip(times, dictionaries):
assert expected == td.get_arguments()

for expected, dictionary in zip(times, dictionaries):
assert expected == Timedelta.from_dictionary(dictionary)


def test_relative_month():

This comment has been minimized.

Copy link
@kmax12

kmax12 Aug 15, 2019

Member

can you add a test case for using the plural of months as well? e.g "6 months"

This comment has been minimized.

Copy link
@jeremyliweishih

jeremyliweishih Aug 15, 2019

Author Contributor

Added!

@kmax12
kmax12 approved these changes Aug 15, 2019
Copy link
Member

left a comment

Ready to merge!

@jeremyliweishih jeremyliweishih merged commit 56048d2 into master Aug 15, 2019

2 checks passed

license/cla Contributor License Agreement is signed.
Details
test_all_python_versions Workflow: test_all_python_versions
Details
@rwedge rwedge referenced this pull request Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.