-
-
Notifications
You must be signed in to change notification settings - Fork 404
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
Add Chord element #2138
Add Chord element #2138
Conversation
Nice! We sure need the automatic viewport padding... |
Since a Chord plot is always drawn on a unit circle I've now added some default padding which can be adjusted by using the |
2541530
to
579927c
Compare
9954701
to
96f86ec
Compare
This PR is now feature complete, and behaves how I think it should. The upcoming 0.12.11 bokeh release will fix a bug updating the colormapping of the edges, which shouldn't be an issue as that will be released long before our next release. It's now just missing reference doc entries and plenty of unit tests. |
f501463
to
4f4e91d
Compare
d7e8a46
to
b259ef3
Compare
6babc85
to
7ce4ef0
Compare
Ready to review and merge. |
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.
Looks great!
"outputs": [], | ||
"source": [ | ||
"links = pd.DataFrame(data['links'])\n", | ||
"print(links.head(3))\n", |
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.
I would prefer to see this cell split:
links = pd.DataFrame(data['links'])
links.head(3)
Then:
hv.Chord(links)
I also notice the kernel metadata is still present in the notebooks. Could you please clear that out?
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.
Sure.
@@ -235,6 +235,23 @@ def circular_layout(nodes): | |||
return (x, y, nodes) | |||
|
|||
|
|||
def quadratic_bezier(start, end, c0=(0, 0), c1=(0, 0), steps=50): |
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.
Ideally this should be handled by the plotting extension. The main advantage I see of us doing this is that it helps consistency between matplotlib/bokeh. That said, plotting libraries might have more performant/robust code (though this is using numpy so it shouldn't be too slow).
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.
I'll aim to make a PR against bokeh that allows using the bezier glyph as the edge renderer on a graph renderer, but that might be a little while.
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.
That would be nice. I might actually prefer holoviews handling this if the available bezier definitions in matplotlib/bokeh don't match up.
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.
Afaik they do match.
@@ -9,6 +9,8 @@ | |||
from holoviews.plotting import comms | |||
|
|||
try: | |||
from matplotlib import pyplot | |||
pyplot.switch_backend('agg') |
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.
I am assuming this isn't directly related to this PR?
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.
Just part of shuffling tests around.
Made a few comments but overall it looks good. Once they are addressed, the merge conflict is resolved and the tests are green, I'm happy to merge. |
5db7888
to
1c1b5b2
Compare
Made the requested change to the notebooks and resolved the merge conflicts. Ready to merge when tests pass. |
1c1b5b2
to
9ce69db
Compare
Looks good. Merging. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
As outlined in #2137 this PR adds support for a
Chord
element. So far this is just a WIP, but basically this just extends the existingGraph
element to lay things out in a circle and draw bezier splines between the nodes. The next step will be to allow scaling the line thickness by a value in a separate PR, which will allow scaling the chords representing the edges between nodes and then start drawing the bands around the circle.Progress for what is supported can be tracked in: https://anaconda.org/philippjfr/chord/notebook