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

Plotting entitysets #382

Merged
merged 20 commits into from
Jan 30, 2019
Merged

Plotting entitysets #382

merged 20 commits into from
Jan 30, 2019

Conversation

floscha
Copy link
Contributor

@floscha 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
Copy link

CLAassistant commented Jan 25, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
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
Copy link
Contributor

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
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
Copy link
Contributor

kmax12 commented Jan 25, 2019

great. someone on our side will review soon.

Copy link
Contributor

@kmax12 kmax12 left a comment

Choose a reason for hiding this comment

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

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!

requirements.txt Outdated
@@ -1,5 +1,6 @@
boto3>=1.9.51
botocore>=1.12.51
graphviz>=0.10.1
Copy link
Contributor

Choose a reason for hiding this comment

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

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')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
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
Copy link
Contributor

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
Copy link
Contributor Author

floscha commented Jan 30, 2019

My bad. They're fixed now

@kmax12
Copy link
Contributor

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 alteryx:master Jan 30, 2019
@rwedge rwedge mentioned this pull request Jan 30, 2019
@kmax12 kmax12 mentioned this pull request Jan 31, 2019
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