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

Plotting entitysets #382

Merged
merged 20 commits into from Jan 30, 2019

Conversation

Projects
None yet
3 participants
@floscha
Copy link
Contributor

floscha commented Jan 25, 2019

This PR adds a plot() method to the EntitySet class as described in #381, as well as a suite of relevant unit tests.

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Jan 25, 2019

CLA assistant check
All committers have signed the CLA.

floscha added some commits Jan 25, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 25, 2019

Codecov Report

Merging #382 into master will decrease coverage by 0.08%.
The diff coverage is 84.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #382      +/-   ##
==========================================
- Coverage   95.85%   95.77%   -0.09%     
==========================================
  Files          89       90       +1     
  Lines        7798     7857      +59     
==========================================
+ Hits         7475     7525      +50     
- Misses        323      332       +9
Impacted Files Coverage Δ
...eaturetools/tests/entityset_tests/test_plotting.py 100% <100%> (ø)
featuretools/entityset/entityset.py 94.2% <70%> (-1.55%) ⬇️

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 f7386e2...24399a9. Read the comment docs.

@kmax12

This comment has been minimized.

Copy link
Member

kmax12 commented Jan 25, 2019

thank you for the contribution - let us know when it's ready for review.

note: don't worry about the failing codecov checks. those seem to be errors. if you update from master, it may fix

@floscha

This comment has been minimized.

Copy link
Contributor Author

floscha commented Jan 25, 2019

Thanks for the hint. I was already having a hard time figuring out how my changes could've increased the number of misses... Other than that, I'd say the PR is ready for review now

@kmax12

This comment has been minimized.

Copy link
Member

kmax12 commented Jan 25, 2019

great. someone on our side will review soon.

floscha and others added some commits Jan 26, 2019

@kmax12
Copy link
Member

kmax12 left a comment

Looking good. Just my comments and couple more things below related to docs

  1. Can you add this to api_reference.rst?
  2. Can you add this using_entitysets.rst as a note somewhere towards the top?
.. note ::

  You can visualize your entity set structure by using the `EntitySet.plot()` method.

If it's a bit confusing working with docs, let me know and I can help.

Thanks for the contribution!

@@ -1,5 +1,6 @@
boto3>=1.9.51
botocore>=1.12.51
graphviz>=0.10.1

This comment has been minimized.

@kmax12

kmax12 Jan 29, 2019

Member

can we make this an optional requirement?

my recommended approach would be to import inside of plot some like this

try:
    import graphviz
except ImportError:
    raise ImportError('Please install graphviz to install use plotting functionality')

This comment has been minimized.

@floscha

floscha Jan 30, 2019

Author Contributor

Fair enough. It's now solved with a779771

# Draw relationships
for rel in self.relationships:
label = '%s -> %s' % (rel._child_variable_id, rel._parent_variable_id)
graph.edge(rel._parent_entity_id, rel._child_entity_id, label=label)

This comment has been minimized.

@kmax12

kmax12 Jan 29, 2019

Member

i believe this edge is going in wrong direction. the arrow should point from the child to the parent.

for example in the attached image the orders entity should points to the customers, rather than the other way around

test

This comment has been minimized.

@floscha

floscha Jan 30, 2019

Author Contributor

You're right regarding the direction. Also, this page suggests that there's a dedicated "one-to-many"-relationship arrowhead, which I also added with 1216fb4 and made all lines orthogonal. Furthermore, if the key is the same for both related entities, the key is now only displayed once.

The result now looks like this:
download

I'm not 100% satisfied with the aesthetics but I guess that'll have to do for now.

floscha added some commits Jan 30, 2019

@floscha

This comment has been minimized.

Copy link
Contributor Author

floscha commented Jan 30, 2019

Thanks for your feedback @kmax12. I've extended the docs with 11ce114 (I hope I've done that part right) and incorporated your comments (see above).

@kmax12

This comment has been minimized.

Copy link
Member

kmax12 commented Jan 30, 2019

@floscha looks like there are a few linting errors being caught in CI. they are

featuretools/entityset/entityset.py:8:1: F401 'graphviz' imported but unused
featuretools/entityset/entityset.py:1111:13: F811 redefinition of unused 'graphviz' from line 8
featuretools/entityset/entityset.py:1136:55: E231 missing whitespace after ':'

you should be able to reproduce locally by running make lint.

@floscha

This comment has been minimized.

Copy link
Contributor Author

floscha commented Jan 30, 2019

My bad. They're fixed now

kmax12 and others added some commits Jan 30, 2019

@kmax12

This comment has been minimized.

Copy link
Member

kmax12 commented Jan 30, 2019

This looks great. I just made a some changes to the docs and reverted the arrow back to what you had before.

Merging now.

@kmax12 kmax12 merged commit db365f9 into Featuretools:master Jan 30, 2019

1 of 3 checks passed

codecov/patch 84.74% of diff hit (target 95.85%)
Details
codecov/project 95.77% (-0.09%) compared to f7386e2
Details
license/cla Contributor License Agreement is signed.
Details

@rwedge rwedge referenced this pull request Jan 30, 2019

Merged

v0.6.0 #387

@kmax12 kmax12 referenced this pull request Jan 31, 2019

Closed

Plotting EntitySets #381

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