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

Unify utility classes for operation on graphs #13

Open
maarzt opened this issue Mar 10, 2023 · 7 comments
Open

Unify utility classes for operation on graphs #13

maarzt opened this issue Mar 10, 2023 · 7 comments

Comments

@maarzt
Copy link
Contributor

maarzt commented Mar 10, 2023

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

@maarzt maarzt mentioned this issue Mar 10, 2023
15 tasks
@xulman
Copy link
Collaborator

xulman commented Mar 12, 2023

@stefanhahmann
Copy link
Collaborator

Just as a reminder: (mastodon-sc/mastodon#229)

grafik

@tinevez
Copy link
Contributor

tinevez commented Aug 21, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

xulman commented Aug 21, 2023

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
Copy link
Contributor Author

maarzt commented Sep 7, 2023

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
Copy link
Contributor

tinevez commented Sep 7, 2023

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

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

No branches or pull requests

4 participants