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

Update the logic of edge attribute generation #521

Merged

Conversation

fenguoerbian
Copy link
Contributor

Previously, edge attribute is generated based on whether colnames(edges_df)[j] is in selected attributes and the content for current edge edges_df[i, j] is valid.

The checks on edges_df[i, j] is whether it is NA or not empty string. But this logic is not closed since edges_df[i, j] can be empty string (''). This is possible since in previous code in generate_dot(), empty attributes have been set to empty string, not NA. This will lead to some bad behavior:

  1. If the 1st edge does not have attributes set, this function will throw a "object 'attribute' not found" error. Rendering issue in PowerBI  #440

  2. If the previous edge has some attributes set while latters don't, then the previous attributes will be carried over to latter edges since attribute is not properly set to NULL at the beginning for each edge. e.g.

library(DiagrammeR)

graph <- create_graph() %>%
    add_n_nodes(n = 3) %>%
    add_edge(from = 1, to = 2, edge_aes = edge_aes(label = "a")) %>%
    add_edge(from = 2, to = 3)

graph %>%
    generate_dot() %>%
    cat()
#> digraph {
#> 
#> graph [layout = 'neato',
#>        outputorder = 'edgesfirst',
#>        bgcolor = 'white']
#> 
#> node [fontname = 'Helvetica',
#>       fontsize = '10',
#>       shape = 'circle',
#>       fixedsize = 'true',
#>       width = '0.5',
#>       style = 'filled',
#>       fillcolor = 'aliceblue',
#>       color = 'gray70',
#>       fontcolor = 'gray50']
#> 
#> edge [fontname = 'Helvetica',
#>      fontsize = '8',
#>      len = '1.5',
#>      color = 'gray80',
#>      arrowsize = '0.5']
#> 
#>   '1'
#>   '2'
#>   '3'
#> '1'->'2' [label = 'a'] 
#> '2'->'3' [label = 'a'] 
#> }

This patch adds a new check for edges_df[i, j] being empty string to fix this loop hole.

Previously, edge attribute is generated based on whether `colnames(edges_df)[j]`
is in selected attributes and the content for current edge `edges_df[i, j]` is
valid.

The checks on `edges_df[i, j]` is whether it is NA or not empty string. But this
logic is not closed since `edges_df[i, j]` can be empty string (''). This is
possible since in previous code in `generate_dot()`, empty attributes have been
set to empty string, not NA. This will lead to some bad behavior:

1. If the 1st edge does not have attributes set, this function will throw a "'
attribute' not found" error.

2. If the previous edge has some attributes set while latters don't, then the
previous attributes will be carried over to latter edges since `attribute` is
not properly set to `NULL` at the beginning for each edge.

This patch adds a new check for `edges_df[i, j]` being empty string to fix this
loop hole.
Copy link
Owner

@rich-iannone rich-iannone left a comment

Choose a reason for hiding this comment

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

LGTM!

@rich-iannone
Copy link
Owner

@fenguoerbian Thank you for this contribution and sorry for the long wait in reviewing this.

@rich-iannone rich-iannone merged commit f875038 into rich-iannone:main Dec 10, 2024
9 checks passed
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.

2 participants