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

Fix setting edge colors for Chord graphs using an explicit mapping #3734

Merged
merged 1 commit into from
Jul 26, 2019

Conversation

schumann-tim
Copy link
Contributor

and normalization (which is turned on by default).

Details:
The color map's vmin/vmax was computed based on factors of the whole range
whereas the palette only used the factors from the actually used values.

When using color normalization this lead to:
-- LineCollection normalizing via vmin/vmax computed above, reflecting the
whole range.
-- ListedColormap is transforming the normalized color values provided by the
LineCollection using its own size (N). However that size N was based on the
actually used values and hence too small.

and normalization (which is turned on by default).

Details:
The color map's vmin/vmax was computed based on factors of the whole range
whereas the palette only used the factors from the actually used values.

When using color normalization this lead to:
-- LineCollection normalizing via vmin/vmax computed above, reflecting the
   whole range.
-- ListedColormap is transforming the normalized color values provided by the
   LineCollection using its own size (N). However that size N was based on the
   actually used values and hence too small.
@philippjfr philippjfr added the type: bug Something isn't correct or isn't working label May 26, 2019
@philippjfr
Copy link
Member

Thanks for the fix, it's very much appreciated.

@philippjfr
Copy link
Member

Presumably for the bokeh backend this was already handled correctly?

@schumann-tim
Copy link
Contributor Author

Presumably for the bokeh backend this was already handled correctly?
I didn't try it out with bokeh; when I installed holoviews with conda, apparently a too-old version of bokeh was bundled. As I primarily wanted to generate the graphs as svgs I didn't follow up. I can check though.

Would a minimal example be useful for regression testing? (not sure how the current set-up of the project is).

@philippjfr
Copy link
Member

Would a minimal example be useful for regression testing?

Definitely, you can look at the existing tests in holoviews/tests/plotting/matplotlib/testgraphplot.py TestMplChordPlot for some examples.

@schumann-tim
Copy link
Contributor Author

To keep the conversation going, here's a repro for the bug.
I'm unfortunately traveling without access to my computer. Will pick this up when i'm back in ~2 weeks.

import os
import pandas as pd
import holoviews as hv
from holoviews import opts, dim

hv.extension('matplotlib')

edges = [{'source': 0, 'target': 2, 'value': 168},
{'source': 0, 'target': 3, 'value': 150},
{'source': 1, 'target': 3, 'value': 50},
{'source': 1, 'target': 2, 'value': 100}]

nodes = [{'index': 0, 'group': 'clonal_id', 'name': 'grey'},
{'index': 1, 'group': 'clonal_id', 'name': 'red'},
{'index': 2, 'group': 'hits', 'name': 'blue'},
{'index': 3, 'group': 'hits', 'name': 'green'}]

Edges reference by the source's node-id -- so include them as well.

color_map = {'grey':'grey', '0': 'grey',
'red': 'red', '1': 'red',
'blue': 'blue', '2': 'blue',
'green': 'green', '3': 'green'}

links_df = pd.DataFrame(edges)
nodes_ds = hv.Dataset(pd.DataFrame(nodes), 'index')
graph = hv.Chord((links_df, nodes_ds), kdims=['source', 'target']).opts(
opts.Chord(
cmap=color_map, edge_cmap=color_map,
edge_color=(dim('source')).astype(str), label_index='name',
node_color=dim('name').str(), normalize=True))

output_path = '/tmp/repro.pdf'
hv.save(graph, output_path)

@philippjfr
Copy link
Member

Just going to merge.

@philippjfr philippjfr merged commit 640e96c into holoviz:master Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't correct or isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants