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 clustering feature has been dropped accidentally #102

Closed
bburns632 opened this issue Jul 2, 2018 · 5 comments
Closed

node clustering feature has been dropped accidentally #102

bburns632 opened this issue Jul 2, 2018 · 5 comments
Labels

Comments

@bburns632
Copy link
Collaborator

It seems as though node clustering has been removed as a feature in one of our recent PRs. Whoops.

We need to:

  1. Reenable it
  2. write a unit test to check it moving forward.

It was a minor feature, so not an urgent issue. However, it should be PR'd back in before v0.3

@bburns632 bburns632 added the bug label Jul 2, 2018
@jayqi
Copy link
Collaborator

jayqi commented Jul 31, 2018

It looks like this line expects self$orphanNodes in camel case, but the active binding orphan_nodes here is in snake case. And thanks to R's $ NULL behavior, it goes through silently.

@bburns632 bburns632 added this to the Release v0.3.0 milestone Nov 16, 2018
@jayqi
Copy link
Collaborator

jayqi commented Nov 18, 2018

@bburns632 -- I have been thinking: this orphan node clustering implementation is fairly janky, and from what I've seen, doesn't actually improve the layout of the graphs at all (it's all spaced as if the orphan nodes were still there).

I think a better solution would be to set a threshold at which to remove the orphan nodes from the plot entirely.

@bburns632
Copy link
Collaborator Author

@jayqi Having seen a number of large graphs with 10+ orphan nodes, I agree the current clustering solution does not improve the layout of the graph. The graph is still sized with the original nodes included. However, I don't think eliminating them entirely from the report graph fits withe the goal of giving a full picture of a package.

Let's explore hybrid solutions, maybe where orphan nodes are displayed in a separate pane or bullet pointed outside the visualization.

This should only be an issue with the function graph, not the dependency graph.

@jameslamb
Copy link
Collaborator

Agree with everything @bburns632 said. I think the biggest thing that annoys me about current orphan node handling is that we lay them all out horizontally, meaning that having a bunch of orphaned things leads to a tiny hard-to-read plot.

The function and dependency graphs are fundamentally different graphs...dependency graph is a tree while the function graph is just a directed graph with an arbitrary number of connected components.

I think that for orphan nodes, we should set a width in in # nodes and then stack on the right like this (assume node_width = 4):

1 orphan

                                   *
<rest_of_graph>

5 orphans

                                    *
                            * * * *
<rest_of_graph> 

23 orphan nodes

                             * * *
                            * * * *
                            * * * *
                            * * * *
                            * * * *
<rest_of_graph> 

^ nodes didn't line up perfectly, but you get the point

@jayqi
Copy link
Collaborator

jayqi commented Nov 30, 2018

I played around with some other igraph layouts. I think it doesn't make sense to use hierarchical layouts for FunctionReporter and if we use something different, then we don't necessary need to do anything special for the orphan nodes.

See for lubridate:

layout_as_circle
layout_as_circle

layout_with_graphopt
layout_with_graphopt

layout_with_dh
layout_with_dh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants