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

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

Merged
merged 51 commits into from Sep 30, 2019

Conversation

@kmax12
Copy link
Contributor

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

kmax12 added 30 commits Jun 30, 2019
kmax12 added 2 commits Jul 17, 2019
@codecov

This comment has been minimized.

Copy link

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 added 4 commits Jul 19, 2019
@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 added 2 commits Jul 23, 2019
@kmax12 kmax12 changed the base branch from update-progress-bar to master Aug 15, 2019
kmax12 and others added 8 commits Aug 15, 2019
@@ -480,7 +481,7 @@ def test_concat_entitysets(es):
assert old_es_1.__eq__(es_1, deep=True)
assert old_es_2.__eq__(es_2, deep=True)

assert es_3.__eq__(es, deep=True)
assert es_3.__eq__(es)

This comment has been minimized.

Copy link
@rwedge

rwedge Sep 25, 2019

Collaborator

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

This comment has been minimized.

Copy link
@kmax12

kmax12 Sep 26, 2019

Author Contributor

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

kmax12 and others added 5 commits Sep 26, 2019
@rwedge
rwedge approved these changes Sep 30, 2019
Copy link
Collaborator

left a comment

Looks good

@kmax12 kmax12 merged commit 081b874 into master Sep 30, 2019
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 97.77%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +2.22% compared to 6c7aa5c
Details
license/cla Contributor License Agreement is signed.
Details
test_all_python_versions Workflow: test_all_python_versions
Details
@rwedge rwedge deleted the reindex branch Sep 30, 2019
@rwedge rwedge referenced this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.