Skip to content

Change feature calculation to return in order of instance ids provided#676

Merged
kmax12 merged 51 commits intomasterfrom
reindex
Sep 30, 2019
Merged

Change feature calculation to return in order of instance ids provided#676
kmax12 merged 51 commits intomasterfrom
reindex

Conversation

@kmax12
Copy link
Copy Markdown
Contributor

@kmax12 kmax12 commented Jul 17, 2019

This PR updates ft.calculate_feature_matrix to return rows in the order of the instance ids provided.

It also updates entity loading to only sort the input dataframe if a time_index is present. This ensures rows will be returned in the same order as the input if no time index is provided and no instance ids are provided

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 17, 2019

Codecov Report

Merging #676 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #676      +/-   ##
==========================================
- Coverage   97.77%   97.76%   -0.02%     
==========================================
  Files         119      119              
  Lines       10870    10861       -9     
==========================================
- Hits        10628    10618      -10     
- Misses        242      243       +1
Impacted Files Coverage Δ
featuretools/entityset/entity.py 95.63% <ø> (-0.46%) ⬇️
featuretools/tests/synthesis/test_dfs_method.py 100% <100%> (ø) ⬆️
...aturetools/tests/primitive_tests/test_agg_feats.py 99.41% <100%> (-0.01%) ⬇️
...etools/tests/entityset_tests/test_serialization.py 98.08% <100%> (ø) ⬆️
featuretools/tests/entityset_tests/test_es.py 100% <100%> (ø) ⬆️
featuretools/tests/entityset_tests/test_entity.py 100% <100%> (ø) ⬆️
...computational_backends/calculate_feature_matrix.py 98.23% <100%> (ø) ⬆️
...utational_backend/test_calculate_feature_matrix.py 99.37% <100%> (ø) ⬆️

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 6c7aa5c...29406ba. Read the comment docs.

@kmax12 kmax12 changed the title [wip] Change feature calculation to return in order of instance ids provided Change feature calculation to return in order of instance ids provided Jul 19, 2019
@kmax12 kmax12 changed the base branch from update-progress-bar to master August 15, 2019 14:27
assert old_es_2.__eq__(es_2, deep=True)

assert es_3.__eq__(es, deep=True)
assert es_3.__eq__(es)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is due to no longer sorting data for entities without time indexes, we could potentially not split up the data of non-time entities and then we could still do a deep comparison

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

below, we check for the fact that the dataframes have the same data after concatenation, so I think it's fine to just remove the deep check here and not mess with the test

Copy link
Copy Markdown
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

@kmax12 kmax12 merged commit 081b874 into master Sep 30, 2019
@rwedge rwedge deleted the reindex branch September 30, 2019 16:46
@rwedge rwedge mentioned this pull request Sep 30, 2019
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