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

Reingold-Tilford layout gives overlapping nodes #879

Closed
szhorvat opened this issue Oct 1, 2015 · 19 comments
Closed

Reingold-Tilford layout gives overlapping nodes #879

szhorvat opened this issue Oct 1, 2015 · 19 comments
Assignees
Labels
todo Triaged for implementation in some unspecified future version
Milestone

Comments

@szhorvat
Copy link
Member

szhorvat commented Oct 1, 2015

I have a tree on which igraph_layout_reingold_tilford gives overlapping nodes.

Here's what I get:

image

Notice the overlaps on the right.

I'm calling the function as

igraph_layout_reingold_tilford(&graph, &coord, IGRAPH_ALL, NULL, NULL);

Here's the edge list of the graph, in the same format expected by the C interface. Sorry about not being able to come up with a smaller one.

64, 0, 64, 1, 4, 2, 4, 3, 133, 4, 133, 5, 77, 6, 77, 7, 44, 8, 44, 9, 100, 10, 100, 11, 14, 12, 14, 13, 16, 14, 16, 15, 68, 16, 68, 17, 73, 18, 73, 19, 72, 20, 72, 21, 87, 22, 87, 23, 86, 24, 86, 25, 139, 26, 139, 27, 75, 28, 75, 29, 108, 30, 108, 31, 34, 32, 34, 33, 36, 34, 36, 35, 38, 36, 38, 37, 40, 38, 40, 39, 78, 40, 78, 41, 136, 42, 136, 43, 67, 44, 67, 45, 48, 46, 48, 47, 50, 48, 50, 49, 52, 50, 52, 51, 54, 52, 54, 53, 56, 54, 56, 55, 58, 56, 58, 57, 147, 58, 147, 59, 62, 60, 62, 61, 83, 62, 83, 63, 84, 64, 84, 65, 85, 66, 85, 67, 71, 68, 71, 69, 81, 70, 81, 71, 89, 72, 89, 73, 142, 74, 142, 75, 134, 76, 134, 77, 105, 78, 105, 79, 97, 80, 97, 81, 91, 82, 91, 83, 93, 84, 93, 85, 122, 86, 122, 87, 123, 88, 123, 89, 95, 90, 95, 91, 109, 92, 109, 93, 99, 94, 99, 95, 101, 96, 101, 97, 107, 98, 107, 99, 102, 100, 102, 101, 125, 102, 125, 103, 111, 104, 111, 105, 113, 106, 113, 107, 119, 108, 119, 109, 115, 110, 115, 111, 117, 112, 117, 113, 121, 114, 121, 115, 151, 116, 151, 117, 135, 118, 135, 119, 127, 120, 127, 121, 140, 122, 140, 123, 132, 124, 132, 125, 129, 126, 129, 127, 130, 128, 130, 129, 137, 130, 137, 131, 138, 132, 138, 133, 141, 134, 141, 135, 143, 136, 143, 137, 144, 138, 144, 139, 145, 140, 145, 141, 149, 142, 149, 143, 146, 144, 146, 145, 148, 146, 148, 147, 150, 148, 150, 149, 152, 150, 152, 151

@ntamas
Copy link
Member

ntamas commented Oct 1, 2015

Thanks, this is a known issue but the problem is that I never managed to come up with an example graph that is small enough so I could start debugging it. This one's also a bit on the large side - it would be great if it could be made a bit smaller. In the worst case, I'll start removing nodes one by one to make it smaller.

@szhorvat
Copy link
Member Author

szhorvat commented Oct 1, 2015

Is my assumption correct that two nodes should never be closer than distance 1? I.e. the below one is incorrect too, right?

image

I'm trying to generate random trees to find smaller examples.

@ntamas
Copy link
Member

ntamas commented Oct 1, 2015

There seems to be a minsep variable in the Reingold-Tilford code which defines the minimum separation between subtrees. It is set to constant 1 so this tree is probably also incorrect.

@szhorvat
Copy link
Member Author

szhorvat commented Oct 1, 2015

This one above has 21 nodes. The smallest ones where I can find problems are of size 20 (but those don't show the problem as clearly, visually). This one is:

0, 1, 2, 1, 3, 0, 4, 2, 5, 3, 6, 3, 7, 5, 8, 2, 9, 1, 10, 6, 11, 5, 12, 9, 13, 3, 14, 12, 15, 10, 16, 9, 17, 15, 18, 10, 19, 9, 20, 6

@szhorvat
Copy link
Member Author

szhorvat commented Oct 1, 2015

These are a few plots of trees where nodes get closer than 1. What they all have in common is that there's a small subtree in the middle separating two larger subtrees on the left and right. This might help construct a small example manually.

https://dl.dropboxusercontent.com/u/38623/bad-trees.pdf

@szhorvat
Copy link
Member Author

szhorvat commented Oct 1, 2015

OK, here's a test case:

We first start with this, based on the idea from above:

image

Mathematica, which I suspect to use a closely related algorithm, renders this indistinguishably.

Now we start adding nodes to one of the outer subtrees:

image

Now the two leaves in the centre are closer than 1. As a comparison, Mathematica does something slightly different here:

image

Finally, let's add one more leaf:

image

And we have the incorrect rendering!

For this example I set the root vertex manually instead of letting the algorithm choose it.

For comparison, Mathematica does this:

image

Here's the edge list I entered manually. 0 is the root.

0, 1,
0, 2,
0, 3,
2, 4,
2, 5,
1, 6,
6, 7,
6, 8,
6, 9,
3, 10,
10, 11,
10, 12,
10, 13,
10, 14,
10, 15

@ntamas
Copy link
Member

ntamas commented Oct 2, 2015

Thanks a lot, great, this is something that I could actually start investigating (and I guess this is probably as minimal as it gets).

@stale
Copy link

stale bot commented Jan 22, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issues that have been inactive for a long time; will be closed in the absence of further activity label Jan 22, 2020
@ntamas ntamas added todo Triaged for implementation in some unspecified future version and removed stale Issues that have been inactive for a long time; will be closed in the absence of further activity labels Jan 22, 2020
@ntamas ntamas added this to the 0.8.3 milestone Sep 14, 2020
@ntamas
Copy link
Member

ntamas commented Sep 14, 2020

I'll try to tackle this before 0.8.3 now that we have a nice reproducible example.

ntamas added a commit that referenced this issue Sep 17, 2020
@ntamas
Copy link
Member

ntamas commented Sep 17, 2020

I have committed a patch here. I think I'm on the right track, but something is still off; this patch fixes the layout of the graphs in this topic, except the very first (large) graph in the first post.

@ntamas
Copy link
Member

ntamas commented Sep 17, 2020

This is the layout that I get now with the patch:

rt_layout

@szhorvat
Copy link
Member Author

szhorvat commented Sep 17, 2020

Here's a 10-vertex example that fails after the patch:

[[7, 9], [5, 9], [6, 7], [3, 9], [1, 3], [3, 8], [4, 6], [2, 3], [0, 4]]

This is an edge list in Python format.

I get this:

image

(note that the vertices are indexed from 1 to 10 in the picture).

I produced this with a random fuzzer. I could not make a smaller example than this, and I could not make any other 10-vetex example. For reference, the fuzzer code:

Do[
 TimeConstrained[
  While[1 == 
    Max@MorphologicalComponents@
      Binarize@Image@
        IGLayoutReingoldTilford[g = IGTreeGame[n], GraphStyle -> "BasicBlack",
          EdgeStyle -> GrayLevel[0, 1]]];
  Print[g],
  60
  ],
 {n, 2, 10}
 ]

@szhorvat
Copy link
Member Author

Another one:

[[1, 2], [1, 7], [6, 7], [1, 4], [1, 5], [0, 1], [1, 8], [3, 7], [6, 9]]

@ntamas
Copy link
Member

ntamas commented Sep 17, 2020

THis is useful, thanks; will keep on debugging tomorrow.

@ntamas
Copy link
Member

ntamas commented Sep 18, 2020

Okay, I think I now have a version that works for all the cases outlined in this issue. The original graph looks like this now:

rt_layout

@ntamas ntamas self-assigned this Sep 18, 2020
@szhorvat
Copy link
Member Author

This one does not work:

[[5, 13], [9, 13], [7, 9], [7, 10], [0, 10], [0, 6], [3, 6], [4, 6], [2, 9], [0, 1], [6, 8], [0, 14], [10, 11], [8, 12], [6, 15]]

image

@szhorvat
Copy link
Member Author

16-vertex:

[[1, 11], [4, 11], [2, 4], [2, 12], [6, 12], [6, 9], [4, 7], [4, 13], [3, 11], [3, 5], [0, 7], [6, 15], [6, 10], [9, 14], [6, 8]]

@ntamas
Copy link
Member

ntamas commented Sep 18, 2020

For the record: pushed another commit that fixes these and many others; the latest counterexample that is not working is [[23, 29], [23, 38], [26, 38], [26, 39], [7, 29], [7, 25], [3, 25], [3, 37], [24, 37], [24, 36], [19, 36], [23, 33], [27, 33], [20, 38], [14, 24], [4, 14], [18, 36], [10, 18], [8, 33], [2, 8], [14, 22], [17, 29], [22, 35], [7, 11], [18, 28], [9, 25], [5, 18], [21, 28], [21, 31], [12, 18], [29, 30], [10, 16], [13, 26], [13, 34], [0, 34], [6, 7], [10, 15], [1, 9], [13, 32]]. I'll have to postpone this until next week.

@ntamas
Copy link
Member

ntamas commented Sep 25, 2020

@szhorvat This counterexample is now also fixed; please let me know if you can find more with your fuzzer. (I'm still not 100% convinced that all branches of the code are okay -- there are two main branches that are mostly symmetric, depending on whether the left or the right subtree ends when placing two subtrees next to each other, and the latest fix involved only one of the branches).

@ntamas ntamas closed this as completed in 137b569 Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
todo Triaged for implementation in some unspecified future version
Projects
None yet
Development

No branches or pull requests

2 participants