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

Make sure index variable comes first #168

Merged
merged 5 commits into from Jun 18, 2018
Merged

Make sure index variable comes first #168

merged 5 commits into from Jun 18, 2018

Conversation

bschreck
Copy link
Contributor

Reorders variable list in Entity.__init__() so that the index variable is at the front of the list.

@@ -22,6 +22,11 @@ def test_is_index_column(es):
assert es['cohorts'].index == 'cohort'


def test_index_at_beginning(es):
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add another test?

I'm thinking create an entity from a DF where the index isn't the first column and make sure that it gets moved to be be the first in the variables list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah makes sense, I'll add that too

# make sure index is at the beginning
index_variable = [v for v in self.variables
if v.id == self.index][0]
self.variables = [index_variable] + [v for v in self.variables
Copy link
Contributor

@kmax12 kmax12 Jun 13, 2018

Choose a reason for hiding this comment

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

do we not care if the order of variables doesn't match the underlying dataframe? I don't believe we assume that anywhere. can you think of any places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why don't we reorder the columns in the dataframe as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

ya, let's do that

@codecov-io
Copy link

codecov-io commented Jun 13, 2018

Codecov Report

Merging #168 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
+ Coverage   92.88%   92.89%   +0.01%     
==========================================
  Files          70       70              
  Lines        7486     7506      +20     
==========================================
+ Hits         6953     6973      +20     
  Misses        533      533
Impacted Files Coverage Δ
featuretools/tests/entityset_tests/test_entity.py 100% <100%> (ø) ⬆️
featuretools/entityset/entity.py 85.47% <100%> (+0.13%) ⬆️

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 488c60f...c4e9754. Read the comment docs.

@kmax12
Copy link
Contributor

kmax12 commented Jun 18, 2018

Looks good, merging

@kmax12 kmax12 merged commit 174c1b2 into master Jun 18, 2018
@rwedge rwedge mentioned this pull request Jun 22, 2018
@rwedge rwedge deleted the index-variable-first branch June 10, 2019 15:57
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.

None yet

3 participants