search

clij / clij2-fft

21 stars 6 forks source link

Refactoring #6

Closed haesleinhuepf closed 4 years ago

haesleinhuepf commented 4 years ago

Hey Brian @bnorthan ,

this is a quite heavy suggestion for refactoring. My goal was to enable end users applying deconvolution to images of any kind without the need to know how to pad or extend it. Hence, I took your ImageJ Ops code for extending and put it into the deconvolve operation. It takes now an input image of any kind, a psf which has to be normalized, an output image and a number of iterations. I also added a clij implementation of the PSF normalization and split out the code for convolution.

Through this refactoring, these two quite simple example scripts work out of the box:

https://github.com/clij/clij2-fft/blob/refactoring/src/main/jython/deconvolve.py https://github.com/clij/clij2-fft/blob/refactoring/src/main/macro/deconvolve.ijm

Would you mind reviewing my changes? The most important changes were moving code from here to here

I would assume it just runs on linux because I didn't change anything in the C++ part or its wrapper, but would you mind testing the scripts in Fiji on your Linux system? I will in the meantime check on Mac.

Thanks again for all your efforts! I think we are very close to having an OpenCL-based deconvolution tool in Fiji/CLIJ which is super easy to use!

Cheers, Robert

bnorthan commented 4 years ago

Hey @haesleinhuepf

This looks good. I have some time today and I am going to work on a GPU pad function. The current 'pad' is actually 'padAndShiftFFTKernel' (that is it pads and shifts the center of the kernel to the corner). We need to rename that and implement a conventional pad for the image. I am planning to add that change to this branch and clean it up a bit then it should be ready for merge.

However if this conflicts with anything you are doing please let me know.

haesleinhuepf commented 4 years ago

Hey @bnorthan ,

you obviously mean this method, right? Go ahead!

I think when I wrote it, I didn't yet fully understand what we are doing. Just for clarification: We extend the original image so that it has the size of the original + some border. In the corners of that border, we put the PSF. Is this a convention by clfft? Curious!

Thanks for your great work!

bnorthan commented 4 years ago

I think when I wrote it, I didn't yet fully understand what we are doing. Just for clarification: We extend the original image so that it has the size of the original + some border. In the corners of that border, we put the PSF. Is this a convention by clfft?

Yes. We extend the original so it has borders that equal the PSF size divided by 2. This is done to avoid the convolution calculation wrapping to the other side. A extra wrinkle is that clfft does not support every size (some FFTs support every size but are much faster at certain sizes), thus we have to extend even a bit further to the next supported size ( see here).

By convention the origin is 0,0 for clfft. We want the center of the PSF at the origin. And the PSF has to be the same size as the extended image so we can apply the convolution theorem in the frequency domain.

I tweaked this branch a little more. I think it is ready to merge, however please free to make further changes as you see appropriate. When you are satisfied go ahead and merge. Please let me know if you need anything else from me.

haesleinhuepf commented 4 years ago

Awesome, thanks @bnorthan ! 🥳