New visualization does not work properly for large plans

matthiasgomolka commented 4 years ago

Prework

[x] Read and abide by drake's code of conduct.
[x] Search for duplicates among the existing issues, both open and closed.
[x] I am reasonably sure this is a genuine bug in drake and most likely not a user error. (If you run into an error and do not know the cause, please submit a "Trouble" issue instead.)
[x] Be considerate of the maintainer's time and make it as easy as possible to troubleshoot any problems you identify. Read here and here to learn about minimal reproducible examples. Format your code according to the tidyverse style guide to make it easier for others to read.

Description

Between version 7.12.0 and 7.12.4 r_vis_drake_graph() got updated and now uses fancy "curly" arrows to connect targets. But this seems to produce unreadable visualizations for large plans. Here are the dependency graphs from version

7.12.0:

7.12.4:

Reproducible example

I'll try to provide a reprex next week.

Expected result

The dependency graph should be spaced such that it is (somwhat) readable (I know it's hard with plans of this size).

Session info

End the reproducible example with a call to sessionInfo() in the same session (e.g. reprex(si = TRUE)) and include the output.

wlandau commented 4 years ago

Yeah, I do realize the new graph layout is a bit of an adjustment. This is actually visNetwork's default behavior, it does not set a 1:1 aspect ratio on its own. In previous versions of drake, I was able to hack the graph to force a 1:1 aspect ratio, but since we started using visHierarchicalLayout() again in #1289, this became impossible. Nothing I can do except maybe revert #1289, which does not seem worth it.

For what it's worth, the hack did comes with a downside. If you force a 1:1 aspect ratio, vertices pile on top of each other and it is impossible to see target names. Without the hack, zooming in interactively still gives a clear picture.

In general, these large graphs are still going to be hard to read. I suggest you consider switching to dynamic branching so the number of nodes in the graph goes down. Dynamic branching also helps with performance. Initialization times for make() and vis_drake_graph() go way down.

wlandau commented 4 years ago

But maybe visNetwork itself has a workaround. I will ask.

wlandau commented 4 years ago

I may revisit this issue, depending on https://github.com/datastorm-open/visNetwork/issues/386.

strazto commented 4 years ago

IF you're interested in a userland workaround, you can always post-process the graphs put out by drake, using visNetwork to suit your dataset.

It'll resemble something like

# I chose 2500 since the value given in #1289 is 250, and I guess 10 times that might be okay for you?
my_level_separation = 2500 

my_graph %>% visNetwork::visHierachicalLayout(levelSeparation = my_level_separation)

drake's defaults for these spacings likely do a better job servicing more usecases than something that would work for yours.

A reasonable value for level separation is probably calculable based on some ratio of n_nodes / n_levels multipled by some constant (n_levels being the number of stages from the leftmost node to the rightmost)

robitalec commented 3 years ago

Thanks @matthewstrasiotto - that just turned my visGraph from:

to:

(Small note: typo in your code chunk, missing an "r")

my_graph %>% visNetwork::visHierarchicalLayout(levelSeparation = my_level_separation)

matthiasgomolka commented 3 years ago

@matthewstrasiotto, @robitalec, thanks, this is super helpful! I added direction = "LR" to the visNetwork::visHierarchicalLayout call to get the usual drake graph direction from left to right.

strazto commented 3 years ago

It might be worth reopening this issue to either include an override in the params, or to make drake calculate a more appropriate separation based on the breadth of each level

wlandau commented 3 years ago

Added a level_separation argument to vis_drake_graph() and render_drake_graph() in https://github.com/ropensci/drake/commit/65bc865153097b4f9455172327e999e436d30a63.

ropensci / drake