Port kernels

[bnorthan]

I've been working on porting kernel generation. At this point still looking for feedback on the design.

So far I've ported the Gaussian kernel and the Log kernel from pre-existing sources (knime and trackmate). Both of these follow the same pattern.

a) the number of dimensions is defined.
b) the radius (or sigma) is defined as a double
c) the size of each dimensions in pixels is determined by the radius

I've tried for a class hierchy that will handle these implementations but is flexible as to future variations.

[dscho]

@bnorthan if you could add to the commit message a note about the relicensing and that it was actually Jean-Yves Tinevez' code, that would be awesome! 👍 from me!

[bnorthan]

Thanks @tinevez. It was definitely "ported" (copy, paste, refactor as needed, test) so I'll add a note about the relicensing. That is very helpful short term. ( Long term @dscho mentioned we should consider an ops-based gpl repository for deconvolution. I think this is a good idea. Perhaps it should be a little more general. Maybe an ops repository geared towards advanced image processing algorithms with gpl license?? ).

[bnorthan]

Renamed 'FromNumDimensions' to 'NDD'. NDD stands for N-dimensional-dynamic. That means the kernel size is determined from the other inputs (for example sigma and calibrations). The reason for the distinction is that in the future there might be a different interface that takes the kernel size explicitly.

Let me know if this makes sense. And how does the rest of it look?? Thanks.

[dscho]

And how does the rest of it look?

Oh, I forgot to say that I only pointed out things that are not brilliant ;-) 👍 from my side!

[ctrueden]

@bnorthan: Is this good to merge? Or is there further work that needs to happen first?

[bnorthan]

It's almost there but I fixed something and added a test. I'll let you
know when it is ready.

On Fri, Sep 19, 2014 at 3:05 PM, Curtis Rueden notifications@github.com
wrote:

@bnorthan https://github.com/bnorthan: Is this good to merge? Or is
there further work that needs to happen first?

—
Reply to this email directly or view it on GitHub
#48 (comment).

[bnorthan]

Hi Curtis - I think this should be reviewed by @dietzc when he gets back home and gets a chance (given his experience with knime filters). A couple of things have just come to mind:

a) this is an extension of create so there will be many variations of source (create from Img, create from ImgPlus, create from Type) and parameters (create from given dimensions, calculate dimension from parameters (such as sigma)). I've only implemented one case. So your plans regarding "converting" ops will be relevant here for other cases.

b) there are a few other subtle issues. For example the Gaussian code was originally written for doubles and integer types would have to be handled a bit differently.

[bnorthan]

I changed things a bit to be compatible with the latest work done at the Hackathon.

a) interfaces now added via the Ops.list file.

b) CreateLog/GaussKernel should really extend a createimg class. I did not see a very simple AbstractCreateImg (create from type and factory then dimensions and values are determined by derived classes) so I added one. Let me know if this makes sense.

[ctrueden]

@bnorthan Just waiting on review from @dietzc, correct?

[ctrueden]

@dietzc Any comments?

[dietzc]

@ctrueden @bnorthan I will review this weekend! I'm sorry for the delay.

[dietzc]

I like it, as it is really thin and straightforward implemented. I added some comments to the commit. Here are some additionally notes:

• In general I would declare the fields in the ops as private whenever possible.
• One more thing we may want to think about is if we really want to use the Gauss interface here or if a GaussKernel interface would be better? I'm a bit worried, that we may mix up the convolution with the actual creation of the kernel.
• Maybe we can provide default values for some of the fields in the case of KernelCreation? For example, if I want to create a Gauss Kernel, the default setting is: ArrayImgFactory + Double?

Hope this helps!

[bnorthan]

Thanks for the feedback @dietzc I should have time to make the updates later this week or early next.

[bnorthan]

@dietzc, @ctrueden

Hi Christian - I am changing the interface to support different sigmas for each dimension but I am getting an error when trying to pass in a double[] Parameter to the op.

It appears to me that it is looking for a 2D double array for the last parameter ([D [D@49d5e6da). Or perhaps I am not understanding the error message?? Have you guys had any problems passing arrays to ops?

I expected the op service to match the "CreateGaussianKernel" op. But for some reason it is reporting that op as {!INVALID}. What does that mean?? That the signature did not match?? Or something else is wrong with the op??

gausskernel(net.imglib2.img.array.ArrayImgFactory net.imglib2.img.array.ArrayImgFactory@321236f4, net.imglib2.type.numeric.real.FloatType 0.0, java.lang.Integer 2, [D [D@49d5e6da)
Candidates:
[100.0](net.imglib2.img.Img<capture of ?> output) = net.imagej.ops.convolve.kernel.create.CreateSymmetricGaussianKernel(net.imglib2.img.ImgFactory<capture of ?> fac, capture of ? outType, int numDimensions, double sigma, [D calibration?)
{!INVALID!} [0.0](net.imglib2.img.Img<capture of ?> output) = net.imagej.ops.convolve.kernel.create.CreateGaussianKernel(net.imglib2.img.ImgFactory<capture of ?> fac, capture of ? outType, [D sigma, [D calibration?)

[dietzc]

it seems that you have to pass two arrays? one for the sigmas and one for the calibration?!

[bnorthan]

@dietzc, @ctrueden Hi Christian - actually it wasn't that... the calibration is an optional parameter. It was something else stupid I did in the AbstractCreateKernel class. So ignore the question I had, everything is good now. Thanks for your feedback. I implemented the changes you suggested I just need to look things over a couple of more times. I'll let you know when it's finalized.

[bnorthan]

@dietzc

Suggested changes made and tested except for this part.

Maybe we can provide default values for some of the fields in the case of KernelCreation? For >example, if I want to create a Gauss Kernel, the default setting is: ArrayImgFactory + Double?

Still thinking about the best way to do this. Are you aware of any examples using default factory and type to create an image??

[dietzc]

not from my side. @ctrueden.

I think with merging these changes (especially the createImg stuff), the example in the README.md will work again. @bnorthan did you check that?

btw: great!!! :-)

[bnorthan]

@dietzc, @ctrueden

The script did not work even with my changes because I only added 'AbstractCreateDefaultImg'. If I add a new create class that derives from 'AbstractCreateDefaultImg' it works.

I called it DefaultCreateDefaultImg. I can change the name if you have a suggestion. It uses FloatType and PlanarFactory for defaults if nothing is specified. I can change this too. Any opinion on what should the defaults be??

This class is here.

https://github.com/imagej/imagej-ops/blob/port_kernels/src/main/java/net/imagej/ops/create/DefaultCreateDefaultImg.java

[dietzc]

Thanks

@bnorthan I would (always) prefer to use ArrayImgFactory over PlanaImgFactory. PlanarImgs are just helpers for easier integration with ImageJ1 but is slower in the general case.

one thing which I just discovered: https://github.com/imagej/imagej-ops/blob/port_kernels/src/main/java/net/imagej/ops/create/AbstractCreateImg.java be unifided with AbstractCreateDefaultImg right? Because AbstractCreateDefaultImg doesn't implement any interface. Then you don't get in the naming hell, because DefaultCreateImg would then use FloatType and ArrayImgFactory. Do I miss something here?

@ctrueden any suggestions?

[dscho]

@dietzc you will have to consider the case where width x height x ? > 2^31. (Hint: this is something SCIFIO had to implement itself because ImgLib2 does not offer that convenience functionality.)

[bnorthan]

@dietzc good idea, I merged the classes as you suggested.

I did not use ArrayImgFactory as the default though. There could be problems with very large images. Of course planar will have problems too if the images are very large and you would need a cell image. So it seems that eventually something is needed like "SmartImgFactory" that creates the right type based on size and available memory... but I don't see that anything like that currently exists in imglib2.

[ctrueden]

My suggestion for default would be to use a heuristic. If the requested image size is small enough, use ArrayImg; if not, use a dynamic cell image like SCIFIO does. (This would be a great thing to work on with @hinerm at the upcoming hackathon!)

And SCIFIO already has such a heuristic!

[ctrueden]

We had to switch the DefaultCreateImg type from float to double, but it is finally merged!

[dietzc]

👍