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

Better test case for normalizing from no time index to time index #1113

Merged
merged 6 commits into from
Aug 20, 2020

Conversation

tamargrey
Copy link
Contributor

Updated test case

Creates an entity set with one entity that does not have a time_index but does have a column of datetimes. That column, time is used via make_time_index as the time index of an entity built from normalization.

@tamargrey
Copy link
Contributor Author

A couple things I'm unsure about:

  • If there's a good way to use our demo or test data to do this, I'm open to it; I just couldn't find an entity that didn't have time_index set but still had a datetime column
  • Along those lines, maybe I'm misunderstanding something, and what I described above isn't the test case we're looking for
  • Should we also test the case where both base_entity.time_index is None and we don't set make_time_index to anything?

@codecov
Copy link

codecov bot commented Aug 17, 2020

Codecov Report

Merging #1113 into main will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1113   +/-   ##
=======================================
  Coverage   98.36%   98.36%           
=======================================
  Files         126      126           
  Lines       13257    13268   +11     
=======================================
+ Hits        13040    13051   +11     
  Misses        217      217           
Impacted Files Coverage Δ
featuretools/tests/entityset_tests/test_es.py 99.85% <100.00%> (+<0.01%) ⬆️

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 802cef8...10c8b3a. Read the comment docs.

@tamargrey tamargrey requested review from rwedge and gsheni August 17, 2020 22:13
Copy link
Contributor

@gsheni gsheni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Update the changelog.

docs/source/changelog.rst Outdated Show resolved Hide resolved
@tamargrey tamargrey force-pushed the test-normalize-from-none branch from 16fec8a to fedc197 Compare August 18, 2020 17:12
@gsheni gsheni self-requested a review August 18, 2020 17:31
Copy link
Contributor

@gsheni gsheni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

docs/source/changelog.rst Outdated Show resolved Hide resolved
@gsheni gsheni self-requested a review August 18, 2020 17:45
gsheni
gsheni previously approved these changes Aug 18, 2020
Comment on lines 971 to 980
df = pd.DataFrame({
"id": [0, 1, 2, 3],
"A": [5, 4, 2, 3],
'time': [datetime(2020, 6, 3), (datetime(2020, 3, 12)), datetime(2020, 5, 1), datetime(2020, 4, 22)]
})
es = ft.EntitySet("es")
es = es.entity_from_dataframe(
entity_id="data",
dataframe=df,
index="id")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the old version of this test is testing both a dask entityset and a pandas one. We could make a parametrized entityset fixture similar to es with this portion of the code, and keep the rest of the code to test the entityset

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in the fixtures so we now have params=['dd_normalize_es', 'pd_normalize_es'] for this test-- let me know if it looks ok

rwedge
rwedge previously approved these changes Aug 19, 2020
Copy link
Contributor

@rwedge rwedge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@tamargrey tamargrey merged commit 5d5b2db into main Aug 20, 2020
@tamargrey tamargrey deleted the test-normalize-from-none branch September 4, 2020 13:56
@tamargrey tamargrey mentioned this pull request Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants