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

Remove tibble warnings in render_graph() #507

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

olivroy
Copy link
Collaborator

@olivroy olivroy commented Nov 16, 2023

Basically, I refactored the code by:

  1. return early if output is vistnetwork. (hence the large diff, no longer to nest the code in a if condition)
  2. Use vectors to access the matrix columns instead of using as_tibble()
  3. We error earlier if the layout is not in one of the allowed values
  4. I take some of the common code for each layout and avoid code duplication, only applying correction for the "circle" layout.
  5. Added tests to verify no warning were produced.

I was also able to simplify a condition

# before 
 y * (((count_nodes(graph) + (0.25 * count_nodes(graph)))) / count_nodes(graph)))
# from what I can tell
# this can be simplified to
y * 1.25
# please double check if I am correct.

I suggest looking at the diff side by side since it doesn't look too good inline.

Rationale

The example in README

library(DiagrammeR)
example_graph <-
  create_graph() %>%
  add_pa_graph(
    n = 50, m = 1,
    set_seed = 23
  ) %>%
  add_gnp_graph(
    n = 50, p = 1/100,
    set_seed = 23
  ) %>%
  join_node_attrs(df = get_betweenness(.)) %>%
  join_node_attrs(df = get_degree_total(.)) %>%
  colorize_node_attrs(
    node_attr_from = total_degree,
    node_attr_to = fillcolor,
    palette = "Greens",
    alpha = 90
  ) %>%
  rescale_node_attrs(
    node_attr_from = betweenness,
    to_lower_bound = 0.5,
    to_upper_bound = 1.0,
    node_attr_to = height
  ) %>%
  select_nodes_by_id(nodes = get_articulation_points(.)) %>%
  set_node_attrs_ws(node_attr = peripheries, value = 2) %>%
  set_node_attrs_ws(node_attr = penwidth, value = 3) %>%
  clear_selection() %>%
  set_node_attr_to_display(attr = NULL)


render_graph(example_graph, layout = "nicely")


Warning message:
The `x` argument of `as_tibble.matrix()` must have unique column names if
`.name_repair` is omitted as of tibble 2.0.0.Using compatibility `.name_repair`.The deprecated feature was likely used in the DiagrammeR package.
  Please report the issue at <https://github.com/rich-iannone/DiagrammeR/issues>.
This warning is displayed once every 8 hours.
Call `lifecycle::last_lifecycle_warnings()` to see where this warning was
generated. 

olivroy added a commit to olivroy/DiagrammeR that referenced this pull request Nov 16, 2023
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.

This is great!

@rich-iannone rich-iannone merged commit 3f04ee1 into rich-iannone:main Nov 16, 2023
9 checks passed
rich-iannone pushed a commit that referenced this pull request Nov 16, 2023
* Refactoring conditions in render_graph

* Refactor `render_graph()` code.

* Lint + Improve test coverage

* Add back WS

* Revert logical changes and put them in #504

* More linting

* small adjustments

* Corrections

* Correct conditions

* WS change revert

* Revert changes made in #507

* WS

* WS
@olivroy
Copy link
Collaborator Author

olivroy commented Nov 16, 2023

@rich-iannone I scanned through the issues quickly, would you mind closing #474, #483, #438, #417, #389, #334, #297, #277 (#477, #501, #480, #473, #470, #421, #363 as duplicate of #475 (keep #475 open, maybe pin it?)

@olivroy olivroy deleted the render-graph branch November 16, 2023 14:29
@rich-iannone
Copy link
Owner

This I can do! I'll start on it now.

@olivroy
Copy link
Collaborator Author

olivroy commented Nov 16, 2023

Thanks
also #278, #366

@rich-iannone
Copy link
Owner

Not sure why I didn't do this sooner but I'm adding you as a collaborator on the repo. That should definitely help with issue management and other repo actions!

@olivroy
Copy link
Collaborator Author

olivroy commented Nov 16, 2023

Fyi, I do not plan to merge the PRs (unless you approve them), I just wanted to see if they passed R CMD CHECK

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