-
Notifications
You must be signed in to change notification settings - Fork 893
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
Plotting entitysets #382
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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 |
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 |
great. someone on our side will review soon. |
There was a problem hiding this 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
- Can you add this to
api_reference.rst
? - 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 |
There was a problem hiding this comment.
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')
There was a problem hiding this comment.
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
featuretools/entityset/entityset.py
Outdated
# 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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:
I'm not 100% satisfied with the aesthetics but I guess that'll have to do for now.
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). |
@floscha looks like there are a few linting errors being caught in CI. they are
you should be able to reproduce locally by running |
My bad. They're fixed now |
This looks great. I just made a some changes to the docs and reverted the arrow back to what you had before. Merging now. |
This PR adds a
plot()
method to theEntitySet
class as described in #381, as well as a suite of relevant unit tests.