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

Node Labels #7

Merged
merged 3 commits into from Aug 3, 2018
Merged

Node Labels #7

merged 3 commits into from Aug 3, 2018

Conversation

ryanpeach
Copy link

I added draw_networkx_nodes_labels to draw_altair.py with the following added parameters:

  • node_label_size : scalar or string
    Size of nodes (default=15). If an array is specified it must be the
    same length as nodelist.

  • node_label_color : color string, or array of floats
    Node color. Can be a single color format string (default='r'),
    or a sequence of colors with the same length as nodelist.
    If numeric values are specified they will be mapped to
    colors using the cmap and vmin,vmax parameters. See
    matplotlib.scatter for more details.

  • node_label : string
    The name of the node attribute to treat as a label.

Limited testing shows that it works as expected, though I haven't yet tested color.

@ryanpeach
Copy link
Author

ryanpeach commented Jul 30, 2018

draw_networkx_edge_labels should be an exact copy, except that the x, y would be replaced by the midpoint of the line (however you calculate that).

That is why I made the parameters all titled node_label_* for the future addition of edge_label_*

@Zsailer
Copy link
Owner

Zsailer commented Jul 30, 2018

@ryanpeach Thank you! This is great!

I'll test this out later this today, but looking at it quickly, it looks great!

@ryanpeach
Copy link
Author

We might be able to simplify this by using text as an encoding I just learned, but I think this gives us more control.

Copy link
Owner

@Zsailer Zsailer left a comment

Choose a reason for hiding this comment

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

Although I'm not a fan of networkx's argument naming--I like your labeling much more!--I think we should follow it exactly. I'd like nx_altair's draw module to mirror networkx's draw module (see here). That way, a user can use their old networkx plotting code with nx_altair.

I think there is a big push to define a visualization grammar for networks happening here. When that stabilizes, we can update nx_altair to follow that grammar.

@@ -253,7 +253,7 @@ def draw_networkx_nodes(
elif alpha is not None:
raise Exception("alpha must be a string or None.")

##### alpha
##### cmap
if isinstance(cmap, str):
Copy link
Owner

Choose a reason for hiding this comment

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

👍

chart=None,
layer=None,
nodelist=None,
node_label_size=15,
Copy link
Owner

Choose a reason for hiding this comment

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

  • node_label_size should be font_size
  • node_label_color should be font_color
  • node_label should be labels.

@@ -281,6 +281,119 @@ def draw_networkx_nodes(

return node_chart

def draw_networkx_nodes_labels(
Copy link
Owner

Choose a reason for hiding this comment

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

Should be draw_networkx_labels

@@ -289,6 +402,9 @@ def draw_networkx(
edgelist=None,
node_size=300,
node_color='red',
node_label=None,
Copy link
Owner

Choose a reason for hiding this comment

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

  • node_label_size should be font_size
  • node_label_color should be font_color
  • node_label should be labels.

@Zsailer
Copy link
Owner

Zsailer commented Aug 1, 2018

@ryanpeach

This is great! Thank you for your work on this.

I submitted a review--mostly just renaming variables. I tested out the code and everything works great. If you have any questions, ping me here.

@Zsailer Zsailer mentioned this pull request Aug 3, 2018
@Zsailer Zsailer merged commit d43b048 into Zsailer:master Aug 3, 2018
@Zsailer
Copy link
Owner

Zsailer commented Aug 3, 2018

This PR was auto-closed because it was merged in PR #10.

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.

None yet

2 participants