Add Hough transform #32

panovr · 2017-02-24T16:10:05Z

This pull request is to port the Hough transform from ImgLib1 to ImgLib2, and also see this issue: imagej/imagej-ops#465

hanslovsky · 2017-02-24T17:53:52Z

Just as a note: The link to issue 465 is broken in your comment. I can c&p the text though.

panovr · 2017-02-24T17:57:24Z

@hanslovsky fixed

ctrueden · 2017-02-24T18:27:39Z

pom.xml

 		</dependency>
+		<dependency>
+			<groupId>net.imglib2</groupId>
+			<artifactId>imglib2-algorithm-gpl</artifactId>


This is a problem. The imglib2-algorithm-gpl component must not be a dependency here, both for legal reasons, as well as logistical/organizational ones.

ctrueden · 2017-02-24T18:27:56Z

pom.xml

+		</dependency>
+		<dependency>
+			<groupId>net.imglib2</groupId>
+			<artifactId>imglib2-ij</artifactId>


This is a problem. We must not depend on ImageJ 1.x here.

ctrueden · 2017-02-24T18:28:19Z

pom.xml

+		<!-- SCIFIO dependencies -->
+		<dependency>
+			<groupId>io.scif</groupId>
+			<artifactId>scifio</artifactId>


We must not depend on SCIFIO here. The imglib2-algorithm component should not need to read/write image formats.

ctrueden · 2017-02-24T18:28:45Z

.gitignore

 /.settings/
 /target/
+
+# IntelliJ


This should be a separate commit, with message like "Ignore IntelliJ metadata files"

ctrueden · 2017-02-24T18:29:13Z

src/main/java/net/imglib2/algorithm/hough/HoughLineTransform.java

+ *
+ * rho = y * sin(theta) + x * cos(theta)
+ * @Override
+ *


Add yourself, as well as the original author(s), as @authors here.

ctrueden · 2017-02-24T18:29:27Z

src/main/java/net/imglib2/algorithm/hough/HoughLineTransform.java

+ * For a given point, then, votes are placed along the curve
+ *
+ * rho = y * sin(theta) + x * cos(theta)
+ * @Override


The @Override annotation makes no sense here.

ctrueden · 2017-02-24T18:30:37Z

src/main/java/net/imglib2/algorithm/hough/HoughLineTransform.java

+    private final double[] theta;
+    private ArrayList<double[]> rtPeaks;
+
+    final private static float computeLength(final long[] position) {


Please apply the ImgLib2 style formatting to this class, so formatting is consistent with the rest of ImgLib2.

ctrueden · 2017-02-24T18:34:51Z

Thanks so much for working on this! It is looking really, really good!

Three main concerns:

Extra dependencies are undesirable. I did not see where you actually need them? Probably they can just be removed?
Style formatting would be nice to match with the rest of ImgLib2. There are Eclipse formatting rules for this, but maybe you want to make IntelliJ version of them and commit it to ImgLib2 repository, to help others who use IntelliJ?
We need unit tests.

If you can address the above three things, I am happy.

tinevez · 2017-02-24T19:39:39Z

I notice that this PR covers hough line transform. Are there plans for hough circle transform?

panovr · 2017-02-24T19:56:24Z

@ctrueden I really appreciate for the detailed code review, thanks!

FIX:

the undesirable dependencies exist because I write a test class for validating. I should delete these.
the imglib2-algorithm-gpl is need for the PickImagePeaks class in HoughTransform.java at this line
I format code using default IntelliJ code format, and I will re-format code to ImgLib2 style.
I will complement unit tests. Any example I can follow? @ctrueden

TODO:

Add a method for generate lines coordinates based on vote images
Add hough circle transform or need others contribution @tinevez

HELP:

I still don't know how to use the method public void setSuppression( final double[] r ) in class PickImagePeaks.java of file PickImagePeaks after the vote space image has been computed if I want to invoke the method public ArrayList< long[] > getPeakList() at this line @tinevez

ctrueden · 2017-02-24T20:12:52Z

the imglib2-algorithm-gpl is need for the PickImagePeaks class

OK. If the implementation would be more complicated without using this class, then I propose to add these Hough classes instead to imglib2-algorithm-gpl, to avoid circular dependency, and to have correct, legal licensing.

I will complement unit tests. Any example I can follow?

There are tons of unit tests across many components. We use JUnit. You could look at e.g. DilationTest in this repository to see how Jean-Yves tests the morphological dilate operation.

hanslovsky · 2017-03-06T20:47:50Z

What exactly is the PickImagePeaks class used for in this case? Is it simply to find local extrema? You could use LocalExtrema instead.

ctrueden · 2017-03-08T16:27:03Z

@panovr Did you try with LocalExtrema yet?

panovr · 2017-03-09T16:56:12Z

@hanslovsky @ctrueden I will investigate LocalExtrema later.

tinevez · 2017-04-06T15:57:51Z

I am taking care of the Hough circle transform.

ctrueden · 2017-04-06T17:36:18Z

@tinevez OK, thank you very much!

imagejan · 2017-06-06T14:21:22Z

@tinevez are you planning to migrate your work in https://github.com/tinevez/CircleSkinner/tree/master/src/main/java/net/imagej/circleskinner/hough to imglib2-algorithm?

tinevez · 2017-06-06T14:25:09Z

Yes, before end of June.

ctrueden · 2017-07-18T17:16:49Z

@tinevez I have a new undergrad student, @gselzer, working on ImageJ Ops and ImgLib2 stuff now. If there is anything he could take off your plate relating to the Hough transform project, just let us know!

tinevez · 2017-07-18T22:04:29Z

Christ I am late everywhere.
@gselzer can we chat over gitter sometime about this?

gselzer · 2017-09-21T21:13:22Z

@tinevez I'm sorry that I did not see this comment before, but I am just now beginning to move onto this project and would love to chat on Gitter!

ctrueden · 2017-11-07T21:17:07Z

See also imagej/imagej-ops#526.

hanslovsky · 2018-01-23T15:45:19Z

@panovr did you have a look at LocalExtrema and whether or not that could (partially) replace PickImagePeaks?

ctrueden · 2018-01-23T17:56:15Z

@hanslovsky FYI, @panovr has moved on from LOCI (back to China, where has main project is machine learning classification and segmentation of bird species). But our student @gselzer could (will! 😉) work on completing this PR.

hanslovsky · 2018-01-23T19:44:39Z

Thanks for the update @ctrueden

gselzer · 2018-05-15T21:42:10Z

In general I do agree and this is what I was referring to with my more high-level test that does not look at the vote space and only checks if the correct parameters are obtained.

Ah okay now I understand what you mean by a high-level test. This is what testPickPeaks currently does; it takes the same image used in testHoughLineTransformToTarget, runs the transform and then pickLinePeaks on the resulting votespace, and then compares the resulting Points to what is expected.

Only if we can comprehend how the ground truth was generated, otherwise a future error will be rather useless. I still don't see how an appropriate example cannot be hand crafted.

I think that the point @ctrueden and I are trying to make here is that this test has less to do with assuring the correctness of the algorithm and more to do with guaranteeing reproducability. This test will fail if any math changes and as such a success from this test is a sign that an output after an update to the algorithm is the same output that would have been given before. With that being said you are right in that it is not an indicator that the algorithm is working correctly; that is what testPickPeaks is designed to do. We can change the name of this test if that would help make it more clear that reproducability is the goal.

hanslovsky · 2018-05-16T12:33:33Z

@gselzer I still think that a hand-crafted test would do the same thing and, on top of that, would add documentation of what is being tested. But @ctrueden is an imglib2 admin and should have the last word on this.

hanslovsky · 2018-05-16T12:43:36Z

src/main/java/net/imglib2/algorithm/hough/HoughTransforms.java

+
+		while ( imageCursor.hasNext() )
+		{
+			double fRho;


No need to declare fRho and r outside the loop. Put declaration at initialization (line 393) and change r to long.

hanslovsky · 2018-05-16T12:45:50Z

src/main/java/net/imglib2/algorithm/hough/HoughTransforms.java

+				for ( int t = 0; t < nTheta; ++t )
+				{
+					fRho = Math.cos( theta[ t ] ) * imageCursor.getDoublePosition( 0 ) + Math.sin( theta[ t ] ) * imageCursor.getDoublePosition( 1 );
+					r = Math.round( ( float ) ( ( fRho - minRho ) / dRho ) );


Do not cast to float before rounding.

Is rounding correct? That would mean that the value of each r bin defines the center of said bin, not the min. If that is the desired behavior, ignore this second bullet point.

@hanslovsky you are right about the cast. I do however think that this rounding is correct since we are using r to approximate our value of rho, meaning that we want to round up in the case that rho is closer to the r greater than it than it is to the r less than it, if that makes sense.

hanslovsky · 2018-05-16T12:48:19Z

src/main/java/net/imglib2/algorithm/hough/HoughTransforms.java

+			{
+				for ( int t = 0; t < nTheta; ++t )
+				{
+					fRho = Math.cos( theta[ t ] ) * imageCursor.getDoublePosition( 0 ) + Math.sin( theta[ t ] ) * imageCursor.getDoublePosition( 1 );


x and y should be cached outside loop to avoid potential virtual method call overhead:

if ( filter.test( imageCursor.get() ) ) { double x = imageCursor.getDoublePosition( 0 ); double y = imageCursor.getDoublePosition( 1 ); for () // continue as before. }

hanslovsky · 2018-05-16T12:52:10Z

src/main/java/net/imglib2/algorithm/hough/HoughTransforms.java

+		final double minTheta = -Math.PI / 2;
+		final double dTheta = Math.PI / nTheta;
+
+		final double[] rho = new double[ nRho ];


double[] rho is never used, can be removed

double[] theta caches theta but the expensive caclulations are Math.sin( theta ) and Math.cos( theta ). Cache these instead.

this commit removes the unused rho[] LUT and refactors the theta[] LUT into two different LUTs for sin(theta) and cos(theta). This refactor speeds up later calculations

This commit changes the Predicate deciding whether to allow votespace population from being a strictly greater-than to a greater-than-or-equal-to comparison. This fix was made to prevent a bug in which no threshold is provided to the algorithm and at the same time the max value of the image is at the first index of the image. If the comparison is not <= then all pixels will fail the Predicate, since no pixel is greater than the max. This means that passing a value to the algorithm is still preferable but allows more scenarios where no value is given.

hanslovsky · 2018-05-16T20:42:47Z

The build currently fails with JavaDoc errors. Most of those errors are unrelated and fixed in master. I locally merged with master and the only remaining JavaDoc error is this:

[ERROR] /path/to/imglib2-algorithm/src/main/java/net/imglib2/algorithm/hough/HoughTransforms.java:325: error: reference not found
[ERROR]          * vote space as well. Thus if {@link HoughTransforms#pickPeaks} is not used

This is an easy fix, just repleace pickPeaks with pickLinePeaks. To confirm that JavaDoc generation does not fail you can run mvn javadoc:javadoc (warnings can be ignored).

hanslovsky · 2018-05-16T20:52:05Z

src/main/java/net/imglib2/algorithm/hough/HoughTransforms.java

+import net.imglib2.view.Views;
+
+/**
+ * This abstract class provides some basic functionality for use with arbitrary


This documentation is not accurate anymore, should probably be something along the lines of:
This class provides static methods for the detection of lines in two-dimensional images via the Hough transform.

hanslovsky · 2018-05-16T20:55:30Z

@gselzer Once you update

the JavaDoc to fix the error, and
my last requested change,

this looks good for a merge to me. Yours and @ctrueden's argument regarding the test case makes sense and I agree that we should leave it as is now.

gselzer · 2018-05-16T21:00:31Z

@hanslovsky I just fixed the error and the requested change in the most recent commit. Thanks for taking all the time and effort to get this project ready to merge!

hanslovsky · 2018-05-16T21:11:56Z

@gselzer I looked over it once more and it seems good to me. Thank you for putting the work in and bearing with me and all my pedantic requests. I will leave this open for a little bit longer (probably until the end of the week) to see if anyone else raises concerns, and then I will merge.

Good work!

hanslovsky · 2018-05-22T11:38:35Z

@imglib/admins What is the appropriate merge strategy? Squash and merge?

ctrueden · 2018-05-22T21:17:00Z

@hanslovsky I personally prefer "Create a merge commit" to preserve the history, assuming each commit compiles with passing tests. Otherwise, what is the point of making clean topic branches? That said, I am not an ImgLib admin—see Governance summary—so ultimately it is up to @tpietzsch and @axtimwalde and @StephanPreibisch.

hanslovsky · 2018-05-22T22:07:04Z

@ctrueden I was advised for my own (single-feature addition) PRs to rebase before a merge but I do not
want to make this decision here.

ctrueden · 2018-05-23T19:54:09Z

I was advised for my own (single-feature addition) PRs to rebase before a merge

Doing that will do a fast-forward merge, which loses the fact that it was ever a branch. This is a bad thing which has caused social confusion and resentment in the past, where fingers were pointed and people were accused of "not working on a branch" even though they did at the time. I strongly encourage the ImgLib2 admins to use "Create a merge commit." This can still be combined with a rebase first, if you want the topic branch to sit on top the latest master. The goal is to avoid fast forward merges.

hanslovsky · 2018-05-23T20:11:56Z

@ctrueden sorry for the confusion. I was advised to rebase my branch, then the PR would be merged with a commit message. That was before github had options to rebase through the web interface.

axtimwalde · 2018-05-23T20:14:24Z

@ctrueden @hanslovsky the history of this pull request is painfully long and of limited educational value. It is certainly possible to ask @gselzer to rebase this post-fact into meaningful dissection supporting units but I also think that we all have a lot of stuff to do and this is not very helpful. I would therefore vote for squash and merge.

ctrueden · 2018-05-23T20:32:21Z

@axtimwalde I don't see why further rebasing would be needed. The commits are already small and clean and granular. It sounds like your beef is really just that there are a lot of them but I think that helps bisectability, rather than harming it. There are no harmful intermediate states, are there? Why not just merge it?

hanslovsky · 2018-08-02T18:44:43Z

@axtimwalde @tpietzsch Should I go ahead and squash and merge?

axtimwalde · 2018-08-02T18:57:02Z

@hanslovsky yes.

hanslovsky · 2018-08-02T19:02:06Z

Thanks @gselzer

ctrueden reviewed Feb 24, 2017

View reviewed changes

panovr force-pushed the hough-transform branch from 3781e52 to 31f7117 Compare March 9, 2017 15:45

ctrueden mentioned this pull request Jul 18, 2017

Add Hough transform imagej/imagej-ops#465

Open

ctrueden assigned gselzer Jan 23, 2018

hanslovsky requested changes May 16, 2018

View reviewed changes

gselzer added 8 commits May 16, 2018 09:08

VoteLines: Translate image for zeroed min

66998bc

VoteLines: Refactor LUTs

3ee8755

this commit removes the unused rho[] LUT and refactors the theta[] LUT into two different LUTs for sin(theta) and cos(theta). This refactor speeds up later calculations

VoteLines: Refactor fRho/r declaration

32f685c

VoteLines: Remove cursor positions from for loop

29df455

VoteLines: remove unnecessary float cast.

abc6829

HoughLineTest: use ArrayImgs to make votespace

8f274a6

HoughLineTest: Formatting

f1188a9

hanslovsky requested changes May 16, 2018

View reviewed changes

VoteLines: Fix javadoc

2e8f6b8

gselzer force-pushed the hough-transform branch from cba4673 to 2e8f6b8 Compare May 16, 2018 20:58

hanslovsky approved these changes May 16, 2018

View reviewed changes

hanslovsky merged commit b19faf9 into imglib:master Aug 2, 2018

Add Hough transform #32

Add Hough transform #32

Uh oh!

Conversation

panovr commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hanslovsky commented Feb 24, 2017

Uh oh!

panovr commented Feb 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctrueden commented Feb 24, 2017

Uh oh!

tinevez commented Feb 24, 2017

Uh oh!

panovr commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ctrueden commented Feb 24, 2017

Uh oh!

hanslovsky commented Mar 6, 2017

Uh oh!

ctrueden commented Mar 8, 2017

Uh oh!

panovr commented Mar 9, 2017

Uh oh!

tinevez commented Apr 6, 2017

Uh oh!

ctrueden commented Apr 6, 2017

Uh oh!

imagejan commented Jun 6, 2017

Uh oh!

tinevez commented Jun 6, 2017

Uh oh!

ctrueden commented Jul 18, 2017

Uh oh!

tinevez commented Jul 18, 2017

Uh oh!

gselzer commented Sep 21, 2017

Uh oh!

ctrueden commented Nov 7, 2017

Uh oh!

hanslovsky commented Jan 23, 2018

Uh oh!

ctrueden commented Jan 23, 2018

Uh oh!

hanslovsky commented Jan 23, 2018

Uh oh!

gselzer commented May 15, 2018

Uh oh!

hanslovsky commented May 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanslovsky commented May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

panovr commented Feb 24, 2017 •

edited

Loading

panovr commented Feb 24, 2017 •

edited

Loading

hanslovsky commented May 16, 2018 •

edited

Loading

hanslovsky commented May 16, 2018 •

edited

Loading