Sobel and hessian filter #349

EikeHeinz · 2016-02-10T19:35:50Z

This adds an implementation of a separated sobel kernel, a sobel filter and a hessian filter. Both filters use a derivative filter that uses the separated sobel kernel. Tests are included.
I've tested the sobel filter on several images and compared these to the already filtered versions.
If required I can add some assert statements to the SobelFilterTest case.

dietzc · 2016-02-17T16:11:29Z

src/main/java/net/imagej/ops/create/CreateNamespace.java

+	}
+
+	@OpMethod(op = net.imagej.ops.create.kernelSobel.CreateKernelSobelSeparated.class)
+	public <T extends ComplexType<T>> Img<T> kernelSobelSeparated(final Type<T> outType) {


can't you use final T outType rather than final Type<T> outType? @ctrueden?

Yes, always use T, not Type<T>. I actually just fixed all of the known offenders with 9316184. Let's try to keep things clean!

dietzc · 2016-02-17T19:28:43Z

src/main/java/net/imagej/ops/filter/hessian/HessianRAI.java

+
+		derivativeComputers = new UnaryComputerOp[in().numDimensions()];
+		for (int i = 0; i < in().numDimensions(); i++) {
+			derivativeComputers[i] = RAIs.computer(ops(), Ops.Filter.DirectionalDerivative.class, in(), i);


only works if DirectionalDerivative is also working for n-d...

dietzc · 2016-02-17T21:36:37Z

@iarganda do you want to review? do the implementations make sense to you? if you have questions about ops, just ping me and I can explain.

iarganda · 2016-02-18T21:37:51Z

Sure, I'll have a look as soon as possible! Thanks!

dietzc · 2016-03-04T09:53:29Z

@iarganda @hornm any comments?

stelfrich · 2016-04-15T09:03:01Z

src/main/java/net/imagej/ops/filter/derivative/PartialDerivativesRAI.java

@@ -0,0 +1,51 @@
+package net.imagej.ops.filter.derivative;


License header is missing.

stelfrich · 2016-04-15T12:09:35Z

Hey @EikeHeinz,
I have added some code-related comments to your latest commits. Some general remarks that I couldn't pin to a specific location in the commits:

Missing license headers
Rename the test to PartialDerivativeFilterTest
Fix the coding style in an individual commit

stelfrich · 2016-04-15T12:10:35Z

src/main/java/net/imagej/ops/create/kernelSobel/CreateKernelSobelSeparated.java

@@ -0,0 +1,69 @@
+package net.imagej.ops.create.kernelSobel;


Missing license header.

EikeHeinz · 2016-04-15T12:15:55Z

Thank you for your comments @stelfrich, I'll look into them as soon as possible.
Note: I think, that the returned CompositeView doesn't contain all derivatives (I'm looking into this). Please keep that in mind, if you are planning to use it.

stelfrich · 2016-04-15T12:16:14Z

src/main/java/net/imagej/ops/filter/derivative/PartialDerivativeRAI.java

+		// add dimensions to kernel if input has more than 2 dimensions to
+		// properly rotate the kernel
+		if (in().numDimensions() > 2) {
+			MixedTransformView<T> expandedKernelA = Views.addDimension(kernelA);


Generalize expandedKernelA to be RandomAccessible<T>.

stelfrich · 2016-04-15T12:35:06Z

Note: I think, that the returned CompositeView doesn't contain all derivatives (I'm looking into this). Please keep that in mind, if you are planning to use it.

I won't be using it until some time next week. No worries 😄

ctrueden · 2016-04-22T15:49:00Z

Thanks everyone for reviewing.

Regarding the license headers, I am less worried about these, since they can be easily added after the fact using mvn license:update-file-header. The other comments, though, would be good to resolve. I especially would like to see no WIP commits in the commit history. Every commit needs to compile with passing tests.

stelfrich · 2016-05-04T09:07:08Z

I stumbled across an issue with using PartialDerivativesRAI on the shape-index-map branch. There, I am reusing an instance of PartialDerivativesRAI for the second-order partial derivatives (see DefaultHessian#L95). When used the second time, the input will be of type ConvertedRandomAccessibleInterval which cannot be cast to Img in PartialDerivative#createOutput.

I have added a dirty hack on that branch to circumvent a matching of CreateImgFromImg to Ops.Create.Img. But doesn't solve the underlying issue that Ops.Create.Img is matched too specifically to be reused later on. Maybe @dietzc has an opinion on that?

dietzc · 2016-05-04T11:25:06Z

why do we need to cast it to Img at all? This shouldn't ever be required.

ctrueden · 2016-06-12T03:17:10Z

@EikeHeinz Awesome, thank you!

stelfrich · 2016-07-19T13:18:27Z

@EikeHeinz Have you gotten around to working on the issues?

EikeHeinz · 2016-07-19T13:49:44Z

@stelfrich Yes, I think I have resolved most or all issues, except the one I'm having with the CompositeView. I'll look into that one again tomorrow and hopefully fix it.

EikeHeinz · 2016-07-20T13:04:44Z

@stelfrich Seems like the rebase requires some more work to do. I'm going to solve the new issues over the next few days.

stelfrich · 2016-08-24T08:00:41Z

@EikeHeinz Were you able to resolve the issues and compute all derivatives? If you need a second opinion/review, I'd be glad to help!

dietzc · 2016-08-24T08:18:29Z

did you see: imglib/imglib2-algorithm#29? does this help?

EikeHeinz · 2016-08-31T15:32:46Z

Sorry for the late answer.
@stelfrich yes I've been able to resolve the remaining issues. I'm currently working on some of the points you've mentioned that I haven't fixed until now.
@dietzc Thanks for the info. I'll have a look.

stelfrich · 2016-09-08T10:30:49Z

src/main/java/net/imagej/ops/filter/FilterNamespace.java

+
+	/** Executes the "directional derivative" operation on the given arguments */
+	@OpMethod(op = net.imagej.ops.filter.derivative.PartialDerivativeRAI.class)
+	public <T extends RealType<T>> RandomAccessibleInterval<T> directionalDerivative(final RandomAccessibleInterval<T> in, final int dimension) {


This one and the next method are a left-over from the rename to partialDerivative, I guess? If so, they can now be removed.

EikeHeinz · 2016-09-08T14:11:02Z

@stelfrich Thanks for your comments and help.
@hornm @dietzc I've made a rebase and cleaned up the commit history. Can one of you have a look please?

ctrueden · 2017-12-05T23:41:16Z

Rebased over the latest master to resolve conflicts.

I almost merged this, but there is one inconsistency I want to fix first: is the op name create.kernelSobel Or create.kernelSobelSeparated? We need to make the naming consistent before we can merge it. @EikeHeinz Any preference?

EikeHeinz · 2017-12-06T11:26:39Z

We can name it kernelSobel, because it's the shorter call, but if there are plans to add the not separated Sobel Kernel we might also call it kernelSobelSeparated. I have no preference.

ctrueden · 2017-12-07T16:48:42Z

I tried to research the difference between separable and non-separable Sobel operators, but wikipedia just describes it as separable. I assume the separable version is the original/canonical one? In which case, calling it only kernelSobel here makes sense. If a non-separable version is added, we could call it kernelSobelNonSeparable or some other distinguishing name.

ctrueden · 2017-12-07T16:54:27Z

Woo hoo, merged! Thank you @EikeHeinz!

dietzc mentioned this pull request Feb 11, 2016

Translate TWS core algorithms to ImageJ Ops fiji/Trainable_Segmentation#18

Open

dietzc reviewed Feb 17, 2016
View reviewed changes

EikeHeinz force-pushed the sobel-hessian-filter branch 2 times, most recently from 5b724bb to 1bbbf2d Compare February 17, 2016 17:39

dietzc reviewed Feb 17, 2016
View reviewed changes

stelfrich mentioned this pull request Mar 17, 2016

GeometricOps: Implement Mean Curvature Feature #274

Open

stelfrich reviewed Apr 15, 2016
View reviewed changes

ctrueden added the needs-changes label Apr 22, 2016

ctrueden assigned EikeHeinz Apr 22, 2016

EikeHeinz force-pushed the sobel-hessian-filter branch from a6c18fa to 1791778 Compare July 20, 2016 10:52

stelfrich reviewed Sep 8, 2016
View reviewed changes

EikeHeinz force-pushed the sobel-hessian-filter branch from 85366c0 to d0c573e Compare September 8, 2016 13:51

EikeHeinz force-pushed the sobel-hessian-filter branch from 92fac01 to 4d2292f Compare September 19, 2016 10:36

EikeHeinz force-pushed the sobel-hessian-filter branch 2 times, most recently from 94724f4 to 713f9ce Compare November 7, 2016 13:15

ctrueden force-pushed the sobel-hessian-filter branch from 713f9ce to 9a3d7e8 Compare December 5, 2017 23:39

ctrueden added 2017-12-hackathon needs-discussion and removed needs-changes labels Dec 5, 2017

EikeHeinz added 4 commits December 7, 2017 17:51

add separated sobel kernel

8c6014c

add partial derivative using separated sobel kernel

56d292e

add sobel filter using partial derivative

7acf12c

add hessian filter using partial derivative

8e4061c

ctrueden force-pushed the sobel-hessian-filter branch from 9a3d7e8 to 8e4061c Compare December 7, 2017 16:52

ctrueden merged commit 341758c into master Dec 7, 2017

ctrueden deleted the sobel-hessian-filter branch December 7, 2017 16:54

Uh oh!

Sobel and hessian filter #349

Sobel and hessian filter #349

Uh oh!

Conversation

EikeHeinz commented Feb 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dietzc Feb 17, 2016

Choose a reason for hiding this comment

Uh oh!

ctrueden Feb 17, 2016

Choose a reason for hiding this comment

Uh oh!

dietzc Feb 17, 2016

Choose a reason for hiding this comment

Uh oh!

dietzc commented Feb 17, 2016

Uh oh!

iarganda commented Feb 18, 2016

Uh oh!

dietzc commented Mar 4, 2016

Uh oh!

stelfrich Apr 15, 2016

Choose a reason for hiding this comment

Uh oh!

stelfrich commented Apr 15, 2016 • edited by EikeHeinz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stelfrich Apr 15, 2016

Choose a reason for hiding this comment

Uh oh!

EikeHeinz commented Apr 15, 2016

Uh oh!

stelfrich Apr 15, 2016

Choose a reason for hiding this comment

Uh oh!

stelfrich commented Apr 15, 2016

Uh oh!

ctrueden commented Apr 22, 2016

Uh oh!

stelfrich commented May 4, 2016

Uh oh!

dietzc commented May 4, 2016

Uh oh!

ctrueden commented Jun 12, 2016

Uh oh!

stelfrich commented Jul 19, 2016

Uh oh!

EikeHeinz commented Jul 19, 2016

Uh oh!

EikeHeinz commented Jul 20, 2016

Uh oh!

stelfrich commented Aug 24, 2016

Uh oh!

dietzc commented Aug 24, 2016

Uh oh!

EikeHeinz commented Aug 31, 2016

Uh oh!

stelfrich Sep 8, 2016

Choose a reason for hiding this comment

Uh oh!

EikeHeinz commented Sep 8, 2016

Uh oh!

ctrueden commented Dec 5, 2017

Uh oh!

EikeHeinz commented Dec 6, 2017

Uh oh!

ctrueden commented Dec 7, 2017

Uh oh!

ctrueden commented Dec 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

EikeHeinz commented Feb 10, 2016 •

edited

Loading

stelfrich commented Apr 15, 2016 •

edited by EikeHeinz

Loading