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

Modify retail demo #201

Merged
merged 5 commits into from
Aug 9, 2018
Merged

Modify retail demo #201

merged 5 commits into from
Aug 9, 2018

Conversation

Seth-Rothschild
Copy link
Contributor

We change the underlying CSV of the retail demo to include the following:

  1. Drop duplicate and null rows
  2. Update price to unit_price and add a total column
  3. Mark cancelled orders with a boolean column
  4. Change the unit_price and total values to be in dollars

In addition to those changes to the CSV, there were some changes to the demo.load_retail function:

  1. Add a return_single_table optional argument to match the other demos
  2. Add a dataset source to the docstring as well as a changelog
  3. Move the time index to order_products (which is what it represented) and make new time indices for all of the entities.
  4. Remove country from the customers entity and move it to the orders entity.

@codecov-io
Copy link

codecov-io commented Aug 2, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
+ Coverage   93.35%   93.36%   +<.01%     
==========================================
  Files          70       70              
  Lines        7737     7743       +6     
==========================================
+ Hits         7223     7229       +6     
  Misses        514      514
Impacted Files Coverage Δ
featuretools/tests/demo_tests/test_demo_data.py 100% <100%> (ø) ⬆️
featuretools/demo/retail.py 100% <100%> (ø) ⬆️
featuretools/tests/entityset_tests/test_es.py 99.31% <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 7545ff0...df57f9e. Read the comment docs.

@Seth-Rothschild
Copy link
Contributor Author

Changing this seems to have unexpected consequences on tests. Should not be merged until resolved. Investigating today.

@@ -176,6 +176,7 @@ def test_saveprogress(entityset):
cutoff_time = pd.DataFrame({'time': times, 'instance_id': range(17)})
property_feature = IdentityFeature(entityset['log']['value']) > 10
save_progress = tempfile.mkdtemp()
property_feature.default_value
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default value of property_feature seems to no longer be tested, in spite of no associated changes to that file. I'm testing it directly here, but we should probably find out the root cause. (tagging @rwedge)

@kmax12 kmax12 changed the title Modify retail demo (v2) (WIP) Modify retail demo Aug 6, 2018
@Seth-Rothschild
Copy link
Contributor Author

Seth-Rothschild commented Aug 6, 2018

Edit: Recent commits (8-8-2018) have resolved any remaining testing issues.

The binary transform coverage loss is still completely mysterious, but aside from that the changes are likely ready for inclusion.

The third test in test_calculate_feature_matrix creates a feature matrix with property_feature. In master this triggers generate_default_df while in this branch, with only demo_retail changes, it seems to not.

I should note that whatever is causing this behavior isn't the purpose of the test in question, and should probably be independently tested after we figure out what's going on. Calling property_feature.default_value does hit the missing lines.

@Seth-Rothschild Seth-Rothschild changed the title (WIP) Modify retail demo Modify retail demo Aug 8, 2018
@kmax12
Copy link
Contributor

kmax12 commented Aug 9, 2018

Looks good. Merging

@kmax12 kmax12 merged commit 95d220e into master Aug 9, 2018
@Seth-Rothschild Seth-Rothschild deleted the retail_es_fixes branch August 10, 2018 19:59
@rwedge rwedge mentioned this pull request Aug 20, 2018
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