mastodon-sc / mastodon-tomancak

BSD 2-Clause "Simplified" License
1 stars 5 forks source link

Unify utility classes for operation on graphs #13

Open maarzt opened 1 year ago

maarzt commented 1 year ago

This repository has classes LineageTreeUtils and BranchGraphUtils. These classes should become part of mastodon-collection.

There is also https://github.com/maarzt/mastodon-blender-view/blob/master/mastodon-plugin/src/main/java/org/mastodon/blender/BranchGraphUtils.java

xulman commented 1 year ago

there's also https://github.com/mastodon-sc/mastodon-tomancak/blob/master/src/main/java/org/mastodon/mamut/tomancak/util/SpotsIterator.java that allows to apply operations on visited spots (usage example here or here)

stefanhahmann commented 1 year ago

Just as a reminder: (https://github.com/mastodon-sc/mastodon/pull/229)

grafik

tinevez commented 1 year ago

Hello all,

I am working on this currently and need your input:

LineageTreeUtils.java

The getRoots() method could be replaced by RootFinder.getRoots( graph ), which is more generic and exists already in mastodon-graph. Mathias, would you be ok with this?

The doesBranchDivide() method could be moved to BranchGraphUtils

BranchGraphUtils.java

I think we could make it generic, i.e., working for Vertex and Edge class. I would move it to mastodon-graph in that case, not in mastodon-collection. Ok with this?

SpotsIterator.java

I would wait to move this class. I feel that it could simplified, or even rewritten with calls to more generic methods. For instance the RootFinder.getRoots( graph ) and using the iterator algorithms in org.mastodon.graph.algorithm.traversal. Vlado, what was missing in org.mastodon.graph.algorithm.traversal that made you write the spot iterator?

stefanhahmann commented 1 year ago

Hi @tinevez

I think we could make it generic, i.e., working for Vertex and Edge class. I would move it to mastodon-graph in that case, not in mastodon-collection. Ok with this?

There is also this method, which could be added there (generic already): https://github.com/mastodon-sc/mastodon-deep-lineage/blob/master/src/main/java/org/mastodon/mamut/util/LineageTreeUtils.java

xulman commented 1 year ago

Vlado, what was missing in org.mastodon.graph.algorithm.traversal that made you write the spot iterator?

I don't know... I just didn't know that traversal exists at all... the iterations-traversing I needed was so simple that I didn't care to look around.. just wrapped the usual traversal pattern with Vlado's style convenience methods and the spot iterator class came to being

I don't mind which class is used for the traversing... happy to convert to the algorithm.traversal at some point in the future and throw the SpotsIterator away...


one thing, however, I would love to keep having around... the SpotsIterator was designed with the contract that, yes, it's traversing the graph but either the full graph (when no spots are selected in the Mastodon) or sub-graph (defined by the selected spots in the Mastodon)... so client code's consumer of spots can work either on full graph or just on a arbitrary run-time defined user selection/part of the graph --> super convenient for users if they want to limit the scope of a plugin's operation

maarzt commented 1 year ago

Hello @tinevez,

LineageTreeUtils.java

The getRoots() method could be replaced by RootFinder.getRoots( graph ), which is more generic and exists already in mastodon-graph. Mathias, would you be ok with this?

I see your point. The RootFinder is more generic, it works with any ReadOnlyGraph which is an advantage. But it's also unnecessary complex. It can be replace with a single method:

public static < V extends Vertex< ? > > RefSet< V > getRoots( final ReadOnlyGraph< V, ? > graph )
{
    RefSet< V > roots = RefCollections.createRefSet( graph.vertices() );

    for ( final V v : graph.vertices() )
        if ( v.incomingEdges().isEmpty() )
            roots.add( v );

    return roots;
}

I would suggest to have a GraphUtils class in mastodon-graph with a getRoots and a getLeafs method, and potentially more generally useful methods.

BranchGraphUtils.java

I think we could make it generic, i.e., working for Vertex and Edge class. I would move it to mastodon-graph in that case, not in mastodon-collection. Ok with this?

Yes. Here is a generic version of the BranchGraphUtils class.

SpotsIterator.java

I would wait to move this class. I feel that it could simplified

I agree. I would keep the class in mastodon-tomancak. It would be misplaced in mastodon or mastodon-graph

tinevez commented 1 year ago

Oki! I see that you prepared everything already, for the move to mastodon-graph. Let's do this?