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

Conversation

Projects
None yet
3 participants
@Seth-Rothschild
Contributor

Seth-Rothschild commented Aug 2, 2018

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

Seth-Rothschild commented Aug 3, 2018

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

This comment has been minimized.

@Seth-Rothschild

Seth-Rothschild Aug 3, 2018

Contributor

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 from Modify retail demo (v2) to (WIP) Modify retail demo Aug 6, 2018

@Seth-Rothschild

This comment has been minimized.

Contributor

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 added some commits Aug 6, 2018

@Seth-Rothschild Seth-Rothschild changed the title from (WIP) Modify retail demo to Modify retail demo Aug 8, 2018

@kmax12

This comment has been minimized.

Member

kmax12 commented Aug 9, 2018

Looks good. Merging

@kmax12 kmax12 merged commit 95d220e into master Aug 9, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details

@Seth-Rothschild Seth-Rothschild deleted the retail_es_fixes branch Aug 10, 2018

@rwedge rwedge referenced this pull request Aug 20, 2018

Merged

v0.2.2 #220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment