Corrected Issue with normalized time index and updated testing #768

ablacke-ayx · 2019-10-09T15:27:11Z

Pull Request Description

Fixes #750 - Stop user from removing time index from base_entity using the additional_variables

After creating the pull request: in order to pass the changelog_updated check you will need to update the "Future Release" section of docs/source/changelog.rst to include this pull request.

CLAassistant · 2019-10-09T15:30:48Z

All committers have signed the CLA.

codecov · 2019-10-09T15:36:17Z

Codecov Report

Merging #768 into master will increase coverage by <.01%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master     #768      +/-   ##
==========================================
+ Coverage   97.97%   97.97%   +<.01%     
==========================================
  Files         119      119              
  Lines       10772    10779       +7     
==========================================
+ Hits        10554    10561       +7     
  Misses        218      218

Impacted Files	Coverage Δ
featuretools/entityset/entityset.py	`95.96% <100%> (+0.29%)`	⬆️
featuretools/tests/entityset_tests/test_es.py	`99.8% <85.71%> (-0.2%)`	⬇️

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 235f065...0cdea56. Read the comment docs.

rwedge · 2019-10-10T18:57:59Z

featuretools/tests/entityset_tests/test_es.py

+                            make_time_index='signup_date',
+                            additional_variables=['signup_date'],
+                            copy_variables=[])
+        assert "signup_date" in es["customers"].df.columns


This line will not get hit due to the previous one raising an error

The normalize_entity call will raise an error, which stops code execution within the with statement. If we wanted the final assert statement to be run after the error to confirm we didn't remove the signup date column, we would need to move the assert out of the with pytest.raises block

rwedge · 2019-10-10T19:02:35Z

featuretools/entityset/entityset.py

+
+        for v in additional_variables:
+            if v == base_entity.time_index:
+                raise ValueError("Not moving {} as it is the base time index variable.".format(v))


Let's suggest ways the user can resolve the error, either by setting make_time_index to True so it copies the base entity's time index by default, or by including the variable in copy_variables

rwedge · 2019-10-10T19:04:09Z

docs/source/changelog.rst

@@ -13,7 +14,7 @@ Changelog
    * Testing Changes

    Thanks to the following people for contributing to this release:
-    :user:`kmax12`, :user:`rwedge`
+    :user:`kmax12`, :user:`rwedge`,:user:`ablacke-ayx`


add a space between the comma and your user tag

rwedge

Can you commit a version of the frequently asked questions notebook with the cell outputs cleared?

It's easier to track changes to the contents of the cells when the output is removed.

ablacke-ayx · 2019-10-11T15:30:04Z

Yes. I will on Monday. Sent from my T-Mobile 4G LTE Device Get Outlook for Android<https://aka.ms/ghei36>

…

________________________________ From: Roy Wedge <notifications@github.com> Sent: Friday, October 11, 2019 11:22:10 AM To: FeatureLabs/featuretools <featuretools@noreply.github.com> Cc: Adam Blacke <ablacke@alteryx.com>; Author <author@noreply.github.com> Subject: Re: [FeatureLabs/featuretools] Corrected Issue with normalized time index and updated testing (#768) @rwedge commented on this pull request. Can you commit a version of the frequently asked questions notebook with the cell outputs cleared? It's easier to track changes to the contents of the cells when the output is removed. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_FeatureLabs_featuretools_pull_768-3Femail-5Fsource-3Dnotifications-26email-5Ftoken-3DANDQ3WVUZMWQTBSOF5VBPYDQOCK2FA5CNFSM4I7BBDT2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCHWTRII-23pullrequestreview-2D300759201&d=DwMCaQ&c=xFIy44lPypU_hag4pJs3Dw&r=l-whZnb2vazZNmKwiiJSx7FGX47u3zOCqcQP_21wdwc&m=3sXBTV_wDRNXQDR7k6LNjvibt77MoaPLjcSyN_TeNNw&s=6Km4up7Qh7_jK03QzEckWUUh-vuvKd8RvvpaLQLKqoE&e=>, or unsubscribe<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ANDQ3WW5HUTTGQDNPNYDRBDQOCK2FANCNFSM4I7BBDTQ&d=DwMCaQ&c=xFIy44lPypU_hag4pJs3Dw&r=l-whZnb2vazZNmKwiiJSx7FGX47u3zOCqcQP_21wdwc&m=3sXBTV_wDRNXQDR7k6LNjvibt77MoaPLjcSyN_TeNNw&s=hYCcAXBtRrM5xkSBa7BVOEgmJJu13nMJ54FTC7YlnXM&e=>.

docs/source/frequently_asked_questions.ipynb

rwedge · 2019-10-14T16:57:29Z

docs/source/frequently_asked_questions.ipynb

@@ -238,7 +234,7 @@
    "                         new_entity_id=\"sessions\",\n",
    "                         index=\"session_id\",\n",
    "                         make_time_index=\"session_start\",\n",
-    "                         additional_variables=[\"session_start\"])\n",


I think it is ok to use additional_variables here since in this example "session_start" is not the time index of the base entity

You are right.

rwedge · 2019-10-14T17:00:47Z

featuretools/entityset/entityset.py

        if isinstance(make_time_index, str):
            if make_time_index not in base_entity.df.columns:
                raise ValueError("'make_time_index' must be a variable in the base entity")
-            elif make_time_index not in additional_variables + copy_variables:
-                raise ValueError("'make_time_index' must specified in 'additional_variables' or 'copy_variables'")
+            elif make_time_index not in copy_variables:


This change may prevent moving a column that is not the base entity's time index to the new entity and making it the new time index, which should be allowed

into Issue#750

featuretools/tests/entityset_tests/test_es.py

rwedge · 2019-10-16T15:24:51Z

docs/source/loading_data/using_entitysets.rst

@@ -142,7 +142,7 @@ To finish preparing this dataset, create a "customers" entity using the same met
                             new_entity_id="customers",
                             index="customer_id",
                             make_time_index="join_date",
-                             additional_variables=["zip_code", "join_date"])
+                             additional_variables=["zip_code","join_date"])


there should be a space after the comma on this line

rwedge · 2019-10-16T15:29:55Z

featuretools/tests/entityset_tests/test_es.py

+                        additional_variables=[],
+                        copy_variables=['cancel_date'])
+
+    es = copy.deepcopy(es)


this makes a copy of the newly modified entityset that has a cancellations entity

rwedge

Looks good!

ablacke-ayx added 2 commits October 9, 2019 11:17

Corrected Issue with normatlize time index and updated testing

968f169

updated change log

395c0d7

ablacke-ayx added 6 commits October 9, 2019 11:36

Fixed formating

001f098

Fixed pr number

e9d671a

updated frequently asked questions

fe2e125

fixes to frequently asked questions

28ef180

fixes to using entitysets.rst

51a5830

fixes to using entitysets.rst

0cdea56

rwedge reviewed Oct 10, 2019

View reviewed changes

rwedge reviewed Oct 11, 2019

View reviewed changes

ablacke-ayx and others added 5 commits October 14, 2019 10:07

Updated documentation.

3bc9037

Updated documentation.

3a9b31d

Updated documentation.

94f714e

Updated documentation.

06f6257

remove execution count

0ce970e

rwedge reviewed Oct 14, 2019

View reviewed changes

ablacke-ayx added 3 commits October 14, 2019 15:15

fixed logic error in variable test

8ff22dc

Merge branch 'Issue#750' of https://github.com/featuretools/featuretools

f6fdf0b

into Issue#750

fixed test message in testing.

0c07058

rwedge reviewed Oct 14, 2019

View reviewed changes

featuretools/tests/entityset_tests/test_es.py Show resolved Hide resolved

ablacke-ayx added 5 commits October 15, 2019 11:18

Additional Testing Added.

8f406ec

Additional Testing Added.

33a355a

Corrected object copy

6851bf5

updated rst

91813e4

updated rst

230ff15

reverted examples to prior state.

7f3ebe8

rwedge reviewed Oct 16, 2019

View reviewed changes

ablacke-ayx and others added 4 commits October 16, 2019 15:03

Segmented Tests

30a1ed4

Merge branch 'master' into Issue#750

239af73

clarification of documentation.

df59a3a

clarification of documentation.

21d26f6

rwedge approved these changes Oct 17, 2019

View reviewed changes

ablacke-ayx merged commit c4c7f94 into master Oct 17, 2019

rwedge mentioned this pull request Oct 31, 2019

v0.12.0 #794

Merged

rwedge deleted the Issue#750 branch February 19, 2021 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Corrected Issue with normalized time index and updated testing #768

Corrected Issue with normalized time index and updated testing #768

ablacke-ayx commented Oct 9, 2019

CLAassistant commented Oct 9, 2019 •

edited

Loading

codecov bot commented Oct 9, 2019 •

edited

Loading

rwedge Oct 10, 2019

ablacke-ayx Oct 10, 2019

rwedge Oct 11, 2019 •

edited

Loading

rwedge Oct 10, 2019

rwedge Oct 10, 2019

rwedge left a comment

ablacke-ayx commented Oct 11, 2019 via email

rwedge Oct 14, 2019

ablacke-ayx Oct 14, 2019

rwedge Oct 14, 2019

rwedge Oct 16, 2019

rwedge Oct 16, 2019

rwedge left a comment

Corrected Issue with normalized time index and updated testing #768

Corrected Issue with normalized time index and updated testing #768

Conversation

ablacke-ayx commented Oct 9, 2019

Pull Request Description

CLAassistant commented Oct 9, 2019 • edited Loading

codecov bot commented Oct 9, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwedge Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwedge left a comment

Choose a reason for hiding this comment

ablacke-ayx commented Oct 11, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwedge left a comment

Choose a reason for hiding this comment

CLAassistant commented Oct 9, 2019 •

edited

Loading

codecov bot commented Oct 9, 2019 •

edited

Loading

rwedge Oct 11, 2019 •

edited

Loading