ShowSelectedTracksAction: don't get stuck if there's a loop in the graph #229
Conversation
The "show track downward" action got stuck, whenever there's a loop in the ModelGraph. This is because the "show track downward" action uses the DepthFirstIteration class to iterate over the ModelGraph when searching for the correct new roots. The DepthFirstIteration class itself does not detect loops and hangs if loops are present in the graph. The commit adds the required tests to that keep track of visited nodes and breaks the loop during iteration.
There was a problem hiding this comment.
The pull request fixes the described bug.
I would recommend to add the 2 new notes to the javadoc. It is a private method, but you added javadoc to it anyway already. The notes are very informative and the javadoc would benefit from it.
In general this method seems to be a very efficient way to get the root nodes of selected nodes (and now even avoiding to get stuck in case of loops).
It may even be worth to move this method as a public static method to a Utility class, since it solves a problem that may be used in different context. If you do so, it may be worth to write a unit test for it that shows that it does not fail on trees that have loops.
src/main/java/org/mastodon/views/trackscheme/display/ShowSelectedTracksActions.java
Outdated
Show resolved
Hide resolved
src/main/java/org/mastodon/views/trackscheme/display/ShowSelectedTracksActions.java
Outdated
Show resolved
Hide resolved
|
I extracted the |
This is very good. I noticed that in the class |
src/main/java/org/mastodon/views/trackscheme/display/ShowSelectedTracksActions.java
Outdated
Show resolved
Hide resolved
src/main/java/org/mastodon/views/trackscheme/display/ShowSelectedTracksActions.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for adding findRealRoots() to TreeUtils. I am sure this method can be used elsewhere in the project.
I think the related unit test could be a bit extended. Also this method may deserve some javadoc. I made suggestions for both.
The method findSelectedSubtreeRoots() does not contains the detected code smell anymore and is well reabable.
| final RefList< V > selectedSubtreeRoots = RefCollections.createRefList( graph.vertices() ); | ||
| final RefSet< V > visitedNodes = RefCollections.createRefSet( graph.vertices() ); | ||
|
|
||
| for ( final V realRoot : roots ) |
There was a problem hiding this comment.
I am a bit confused now about what a realRoot is. There is now also a method in this in this class called findRealRoots. In this function a realRoot seems to be something else than within this method. If I understood this correctly, I would think the variable here should be named root instead of realRoot.
There was a problem hiding this comment.
There are reasons to be confused. This PR changes code that works with roots of subtrees. Every node in a tree is at the same time the root of a subtree. So the borders between the terms node and root become blurry. I don't have a good way to resolve this confusion.
There was a problem hiding this comment.
In the mastodon-graph lib there is RootFinder:
https://github.com/mastodon-sc/mastodon-graph/blob/master/src/main/java/org/mastodon/graph/algorithm/RootFinder.java
Maybe we could merge these two?
There was a problem hiding this comment.
I'm not sure what you would like to suggest. Would you like to have a method TreeUtils.getRoots(graph) that replaces new RootFinder(graph).get()?
There was a problem hiding this comment.
Yes exactly. Do you think it is a good fit? One fewer class.... that would be nice.
There was a problem hiding this comment.
I think it would make sense organize the utility classes, but maybe not as a part of this PR.
There is already a ticket mastodon-sc/mastodon-tomancak#13, we should have a look at the RootFinder as part of this.
Rename the findRealRoots to findRootsOfTheGivenNodes. Mastodon works with graphs not trees. The findRootsOfTheGivenNodes method would under some conditions not return all roots of the graph. There is now a unit test for this. And the method has been fixes.
The method findRootsOfTheGivenNodes is used in the ShowSelectedTracksAction. It's purpose is to find the root nodes of the tracks that contain selected nodes. The new implementation gives better results if the ModelGraph also contains merges. The previous implementataion worked fine if the ModelGraph contained only trees. But it did miss root nodes for more general graphs. In order to get all roots it is necessary to do a depth first search that considers the graph as undirected. Example graph: a b | \ | c d If "c" is selected, the method now return both roots "a" and "b" of the track.
fixes #222