-
Notifications
You must be signed in to change notification settings - Fork 20
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
Connected components #54
Conversation
- `ConnectedComponentAlaysis` implementation of connected component analysis - `UnionFind` implementation of union find (required for cca) - `ConnectedComponentAnalaysisTest` tests for 2D and 3D
Thanks to @tpietzsch for a prompt release!
OK to merge @axtimwalde @tpietzsch @StephanPreibisch? |
pom.xml
Outdated
@@ -209,6 +209,7 @@ Jean-Yves Tinevez and Michael Zinsmaier.</license.copyrightOwners> | |||
<dependency> | |||
<groupId>net.imglib2</groupId> | |||
<artifactId>imglib2</artifactId> | |||
<version>4.6.1</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be defined in a property, no?
<properties>
<imglib2.version>4.6.1</imglib2.version>
</properties>
<dependency>
<groupId>net.imglib2</groupId>
<artifactId>imglib2</artifactId>
<version>{imglib2.version}</version>
</dependency>
Besides this, it can probably dropped entirely when switching to the latest pom-imglib2
parent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @imagejan
This was necessary at the time of the PR, but we don't need this anymore, now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need to override the version of a component in the future, please use a version property as @imagejan described, rather than hardcoding it in the <version>
itself. Otherwise, the melting-pot script and other tooling will be unable to override it and some kinds of version skew will not be caught by the unified builds.
I filed scijava/scijava-maven-plugin#16 to hopefully aid with avoiding this scenario in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do (actually, already doing that in most of my projects).
Version specified by pom-scijava is the one we should use
@axtimwalde has a use case in which it would be helpful if connected components analysis would produce the same id as if connected components analysis was run on the full volume for any fully contained connected component in an arbitrary subvolume. As an example, let final RandomAccessibleInterval< B > mask = ...
final RandomAccessibleInterval< B > subVolume1 = Views.interval( mask, ... );
final RandomAccessibleInterval< B > subVolume2 = Views.interval( mask, ... ); Any connected component that is fully contained in I implemented this extension in a separate branch: |
- union find interface to allow for sparse labels - add interface for caller to specify id for each voxel (instead of using IntervalIndexer) Avoid use of streams Add IntArrayUnionFind Add union find for sparse labels with TLongMap as store Add test with offset Expose mapping from set root to id to caller and add IdFromIntervalIndexerWithInterval Restructure CCA
Move test into appropriate package
@axtimwalde I rebased that branch as mentioned above. |
This PR adds a new version of connected component analysis
DiamondShape
with unit rangeExecutorService
required (useful for running wihin parallelization frameworks like Spark)Shape
to allow for arbitrary neighborhoods.Integer.MAX_VALUE
foreground pixels. For large pixels, the task should be divided and parallelized anyway.I suggest we wait until a new release of pom-imglib2 before merging so we can avoid overriding the managed version of imglib2 dependency.