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

fixed #294 - optional id parameter of entityset #324

Merged
merged 1 commit into from Nov 26, 2018

Conversation

Projects
None yet
2 participants
@jeff-hernandez
Collaborator

jeff-hernandez commented Nov 20, 2018

I am not sure if I missed any examples in the documentation. Let me know if anything needs modification. Thanks.

@kmax12

This comment has been minimized.

Member

kmax12 commented Nov 20, 2018

The only place in the codebase I see an entityset's id used is in serialization/deserialization and in the repr methods. Can you also add tests to ensure those work?

also, looks like linting is failing. run make lint locally to see error

@kmax12 kmax12 self-requested a review Nov 20, 2018

@jeff-hernandez jeff-hernandez force-pushed the jeff-hernandez:master branch from accb397 to 5a28cc0 Nov 20, 2018

@codecov

This comment has been minimized.

codecov bot commented Nov 20, 2018

Codecov Report

Merging #324 into master will increase coverage by <.01%.
The diff coverage is 95.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #324      +/-   ##
==========================================
+ Coverage   95.11%   95.12%   +<.01%     
==========================================
  Files          71       71              
  Lines        7683     7705      +22     
==========================================
+ Hits         7308     7329      +21     
- Misses        375      376       +1
Impacted Files Coverage Δ
featuretools/entityset/entityset.py 95.58% <100%> (ø) ⬆️
featuretools/tests/entityset_tests/test_es.py 99.21% <95.45%> (-0.17%) ⬇️

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 624c653...8730984. Read the comment docs.

@jeff-hernandez jeff-hernandez force-pushed the jeff-hernandez:master branch from 5a28cc0 to 2e01425 Nov 21, 2018

@jeff-hernandez

This comment has been minimized.

Collaborator

jeff-hernandez commented Nov 21, 2018

A list of methods of the class EntitySet I found that contain the id property (e.g. self.id):

  • __init__
  • __repr__
  • __getitem__
  • create_metadata_dict

I wrote tests for the methods listed above. I hope that I got all the methods. Let me know if I missed anything. Thanks.

Show resolved Hide resolved featuretools/tests/entityset_tests/test_es.py Outdated
def test_metadata_without_id():
es = ft.EntitySet()
assert es.create_metadata_dict().get('id') is None

This comment has been minimized.

@kmax12

kmax12 Nov 23, 2018

Member

the metadata dict is also used when serializing and loading an entityset. can we add one test that serializes and loads an entityset without an id?

something like

def test_to_pickle_id_none(entityset):
    entityset = ft.EntitySet()
    dirname = os.path.dirname(integration_data.__file__)
    path = os.path.join(dirname, 'test_entityset.p')
    if os.path.exists(path):
        shutil.rmtree(path)
    entityset.to_pickle(path)
    new_es = ft.read_pickle(path)
    assert entityset.__eq__(new_es, deep=True)
    shutil.rmtree(path)

This comment has been minimized.

@jeff-hernandez

jeff-hernandez Nov 26, 2018

Collaborator

Yes not a problem. I made the adjustments described above. Let me know if anything else needs modification. Thanks.

@jeff-hernandez jeff-hernandez force-pushed the jeff-hernandez:master branch from 2e01425 to 8730984 Nov 26, 2018

@kmax12

This comment has been minimized.

Member

kmax12 commented Nov 26, 2018

Looks good. Merging

@kmax12

kmax12 approved these changes Nov 26, 2018

@kmax12 kmax12 merged commit 3d4eff9 into Featuretools:master Nov 26, 2018

3 checks passed

codecov/patch 95.65% of diff hit (target 95.11%)
Details
codecov/project 95.12% (+<.01%) compared to 624c653
Details
license/cla Contributor License Agreement is signed.
Details

@rwedge rwedge referenced this pull request Nov 29, 2018

Merged

v0.4.1 #329

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