-
-
Notifications
You must be signed in to change notification settings - Fork 402
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 Sankey Element #2328
Add Sankey Element #2328
Conversation
7139a13
to
400968c
Compare
Ready to review, going to add unit tests in the morning. |
Here's the reference guide example I used to demonstrate the nodes = ["PhD", "Career Outside Science", "Early Career Researcher",
"Permanent Research Staff", "Professor", "Non-Academic Research"]
nodes = hv.Dataset(enumerate(nodes), 'index', 'label')
edges = [
(0, 1, 53), (0, 2, 47), (2, 5, 17), (2, 3, 30), (3, 1, 22.5), (3, 5, 4.), (3, 4, 0.45)
]
value_dim = hv.Dimension('Percentage', unit='%')
hv.Sankey((edges, nodes), ['From', 'To'], vdims=value_dim).options(
label_index='label', label_position='left', width=800, height=400, edge_color_index='To') |
9f2db0c
to
e2e43ac
Compare
holoviews/core/data/__init__.py
Outdated
@@ -173,7 +173,7 @@ class Dataset(Element): | |||
|
|||
# In the 1D case the interfaces should not automatically add x-values | |||
# to supplied data | |||
_auto_indexable_1d = False | |||
_auto_indexable_1d = True |
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.
Is this correct? The auto indexing behavior caused problems fixed in a recently merged PR...
holoviews/element/sankey.py
Outdated
'which matches the node ids on the edges.') | ||
self._nodes = nodes | ||
chord = layout_sankey(self) | ||
self._nodes = chord.nodes |
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.
Shouldn't be chord
holoviews/element/sankey.py
Outdated
kdims = element.node_type.kdims | ||
nodes = element.node_type(node_data, kdims=kdims, vdims=element.nodes.vdims) | ||
edges = element.edge_type(paths) | ||
sankey = Sankey((element.data, nodes, edges), compute=False) |
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.
It is rather odd how the Sankey
element uses this operation in its constructor which then returns a Sankey
object! It would be better to have a method just computing the information needed by Sankey
and using that in _process
.
holoviews/element/sankey.py
Outdated
_, y0, _, y1 = self.p.bounds | ||
py = self.p.node_padding | ||
|
||
def initializeNodeBreadth(): |
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.
It might be nice to avoid these kinds of inline function - explicit arguments are nicer than closures...
holoviews/element/sankey.py
Outdated
from .util import quadratic_bezier | ||
|
||
|
||
class layout_sankey(Operation): |
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.
Another idea would be to move this code to the element class itself e.g as a number of classmethods used by the constructor to compute the layout. I suppose the parameters of this operation could then be plot parameters. What other use does this operation have other than to create sankey elements which you can just do using Sankey
?
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 mention this as it is a little confusing to see such a long operation in elements
.
@@ -18,6 +18,9 @@ class StatisticsElement(Chart): | |||
|
|||
__abstract = True | |||
|
|||
# Ensure Interface does not add an index | |||
_auto_indexable_1d = False | |||
|
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.
Is this needed?
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.
Yes it is.
holoviews/plotting/bokeh/sankey.py
Outdated
|
||
def get_extents(self, element, ranges): | ||
""" | ||
A Chord plot is always drawn on a unit circle. |
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.
Not a Chord plot!
holoviews/plotting/bokeh/sankey.py
Outdated
|
||
_style_groups = dict(GraphPlot._style_groups, quad='nodes', text='label') | ||
|
||
_draw_order = ['patches', 'multi_line', 'quad', 'text'] |
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.
Is 'multi_line' used?
|
||
def _patch_hover(self, element, data): | ||
""" | ||
Replace edge start and end hover data with label_index data. |
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.
Worth checking if ChordPlot
needs this too...
holoviews/plotting/mpl/__init__.py
Outdated
@@ -83,7 +85,7 @@ def get_color_cycle(): | |||
|
|||
# Define Palettes and cycles from matplotlib colormaps | |||
Palette.colormaps.update({cm: plt.get_cmap(cm) for cm in plt.cm.datad | |||
if 'spectral' not in cm and 'Vega' not in cm}) | |||
if not ('spectral' in cm or (mpl_ge_200 and 'Vega' in cm))}) |
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.
Is this relevant to Sankey
or did this diff come from somewhere else?
@@ -71,7 +71,7 @@ def test_graph_inspection_policy_edges(self): | |||
renderer = plot.handles['glyph_renderer'] | |||
hover = plot.handles['hover'] | |||
self.assertIsInstance(renderer.inspection_policy, EdgesAndLinkedNodes) | |||
self.assertEqual(hover.tooltips, [('start', '@{start}'), ('end', '@{end}')]) | |||
self.assertEqual(hover.tooltips, [('start', '@{start_values}'), ('end', '@{end_values}')]) |
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.
My understanding is these names are a convention established by holoviews for use in bokeh's hover tools. It would be good either to document this in this PR or open an issue as a reminder to document these names.
"edges = pd.DataFrame(data['links'])\n", | ||
"nodes = pd.DataFrame(data['nodes'])\n", | ||
"edges['source'] = nodes['name'].values[edges['source'].values]\n", | ||
"edges['target'] = nodes['name'].values[edges['target'].values]\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.
These two lines are rather obscure. It would be nice if they weren't needed.
I've made a number of comments, most of which are fairly minor (e.g outdated docstrings). The biggest issue right now is that the sankey element is currently more operation than element which is surprising/unusual for something living in the If that is addressed, I am happy to see the PR merged once the tests are passing. |
Looks good. Merging. |
As requested in #1123 this PR implements a Sankey element based on the implementation in d3-sankey. The code consists of three components, which are very similar to the Chord element:
The plots looks pretty much identical across backends. Here is what a simpler example looks like:
Suggestions for reference examples welcome.