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

Normalize entity same index as base entity #681

Merged
merged 6 commits into from
Jul 22, 2019

Conversation

jeremyliweishih
Copy link
Contributor

@jeremyliweishih jeremyliweishih commented Jul 22, 2019

This request addresses #670 .

Adds a check and raises an exception if the index of the base entity is equal to the index argument. This request adds an accompanying test case.

As stated in #670, Feature tools does not handle one-to-one relationships in entitysets. Therefore, this check must be made to stop creating a one-to-one relationship with the same index.

@jeremyliweishih jeremyliweishih changed the title Normalize entity same Normalize entity same index as base entity Jul 22, 2019
@CLAassistant
Copy link

CLAassistant commented Jul 22, 2019

CLA assistant check
All committers have signed the CLA.

@jeremyliweishih jeremyliweishih requested review from rwedge and kmax12 July 22, 2019 20:30
@@ -11,6 +11,7 @@ Changelog
* Set index after adding ancestor relationship variables (:pr:`668`)
* Fix user-supplied variable_types modification in Entity init (:pr:`675`)
* Don't calculate dependencies of unnecessary features (:pr:`667`)
* Normalize entity same index as base entity #681 (:pr:`681`)
Copy link
Contributor

Choose a reason for hiding this comment

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

The ":pr:681" syntax will create a link to this pr, you can remove the "#681". Also can you add a little more to the description so someone unfamiliar could grasp the issue?

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!

@jeremyliweishih jeremyliweishih merged commit 6f4ffd7 into master Jul 22, 2019
@codecov
Copy link

codecov bot commented Jul 23, 2019

Codecov Report

Merging #681 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #681      +/-   ##
==========================================
+ Coverage   97.44%   97.44%   +<.01%     
==========================================
  Files         118      118              
  Lines        9634     9643       +9     
==========================================
+ Hits         9388     9397       +9     
  Misses        246      246
Impacted Files Coverage Δ
featuretools/entityset/entityset.py 95.53% <100%> (+0.02%) ⬆️
featuretools/tests/entityset_tests/test_es.py 100% <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 1bc5f97...f5d9def. Read the comment docs.

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