clij / clij2-fft

21 stars 6 forks source link

CLIJ2-FFT convolve fails silently if raw input image is larger in xy than PSF (kernel) image. #12

Closed chalkie666 closed 3 years ago

chalkie666 commented 3 years ago

Hi folks @haesleinhuepf @bnorthan I'm testing the CLIJ2-fft convolve function pretty hard. I notice that it only seems to work if the input image is smaller in xy than the PSF (kernel) image. It might also be dependent on number of z slices... if they are the same it works, what is working for me is this: raw image 128x128x21 psf 256x256x21

When it doesn't work, there is no error message and it simply returns the input image.

So i can work around it like this, but Obviously biological raw images are usually much larger in xyz than the PSF. So i wonder if its a bug? Looking at the code there is a bunch of image padding etc going on, to get nice images sizes for FFT etc, and maybe I hit an issue with that?

I made a little macro to make this easier to test: https://github.com/chalkie666/imagejMacros/blob/master/DeconvolutionDemos/TestCLIJ2FFTconvolve.ijm

let me know if I can help out in any way, I have time now, I'm on "gardening leave" between jobs., until mid may ish...

I hope to keep working in the longer term future on the 'accelerated" algorithm at: https://github.com/chalkie666/imagejMacros/blob/master/DeconvolutionDemos/AgardAccellVanCittertGoldReferenceImplementation.ijm

So far I got working the Wiener filtering (simpleITK - slow, but only need to do it once) , and the "algebraic" iteration update so now need to do the "geometric" iteration update,
and fine tune the one magic parameter (wiener filter) and test it gives same or similar results to the black box implementation I'm used to using.

One last dumb question: If i want to avoid having to rescale the image intensities of the result of the convolution (adds a step and so loses speed), do I need to normalize the pixel intensities of the PSF somehow? But what to? Sum of all pixels = 1?

cheers

Dan

bnorthan commented 3 years ago

Hi @chalkie666

The convolve should work on every size. I just tested on 216 by 216 img, with 65 by 67 psf (just a random size I cropped from a larger one) and it worked. I also tested a couple of other sizes and it seemed fine. Typical case, as you mention, is image much larger then PSF. We have tested the RL decolution on a variety of image size where image size is >> psf size and haven't seen a problem (2 convolutions are done each RL iteration... though the code path is a bit different)... can you give an example of a size that fails? How much memory does your GPU have?

As to the second question you need to normalize the sum of pixels of the PSF to 1.

Brian

chalkie666 commented 3 years ago

R3Sorry @bnorthan and @haesleinhuepf , I should know better than to not provide a code example showing the issue, and the relevant test data. ;-)

Please see: https://github.com/chalkie666/imagejMacros/blob/master/DeconvolutionDemos/TestCLIJ2FFTconvolve.ijm and the test data is in the same location. The test data are pretty small. 256xy41z for the raw image and 64xy21z for the psf.

I ran it on my geforce GT 730 (2GB) and CPU (32 GB) and got the same results. (look like my GPU started working more reliably...)

Did i read somewhere that the PSF image should have xyz dimensions that are an odd number? (perhaps so the PSF can be centered at a voxel in the middle of the stack? )

thanks for the normalization tip... i was thinking that might be the case, better to do it that way than rescale the result every iteration.

bnorthan commented 3 years ago

Thanks for the images. I was able to repeat the issue right away. That image and PSF actually deconvolved but did not convolve. It turns out the bit of code that extends it to a supported FFT size was only in the deconvolve, and I forgot to put it in convolve. I should be able to get that fixed shortly.

bnorthan commented 3 years ago

Hi @chalkie666

It turns out this fix was already done but has not been included in the latest clijx release. (I did add one small additional change to make the padding more efficient)

@haesleinhuepf how hard is it to create a new release that will include the latest from clij2-fft? If it is a pain I can give @chalkie666 the newest directly.

Brian

bnorthan commented 3 years ago

@chalkie666

What operating system are you using? If it is Linux or Windows I can get you the newest jar and lib that should address the unsupported size issue.

Brian

chalkie666 commented 3 years ago

@bnorthan hi Brian, I'm using Windows10. Didn't install Linux on my workstation yet... its quite new. I would be very happy to test the new things Cheers!

bnorthan commented 3 years ago

Hi @chalkie666 , your macro should work with clij2-fft-2.2.0.14

you can get the files here https://www.dropbox.com/sh/ilup7texs7rsauc/AAAaYJuwUlp22oNVGB7BOA2Ba?dl=0

need to copy everything from lib/win64 to Fiji.app/lib/win64 and over-write

need to copy clij2-fft-2.2.0.14 to Fiji.app/plugins and erase the previous clij2-fft-version.jar (which will have a different version).

haesleinhuepf commented 3 years ago

@haesleinhuepf how hard is it to create a new release that will include the latest from clij2-fft? If it is a pain I can give @chalkie666 the newest directly.

Hi Brian @bnorthan ,

not complicated. But I would like to make sure that the new version is backwards compatible. The signature of DeconvolveRichardsonLucyFFT was changed: https://github.com/clij/clij2-fft/blame/f4fffb7f80b0568c1cd9a90054678fc4b8e07d46/src/main/java/net/haesleinhuepf/clijx/plugins/DeconvolveRichardsonLucyFFT.java#L314

And I'm afraid this will break workflows from users. Workflows which are under review in journals right now. Can we make a second Deconvolve.... method that has more parameters and revert the other so that code is backwards compatible? I can do it if your time is limited. We just need to agree on a name for the new method. :-)

Cheers, Robert

bnorthan commented 3 years ago

Hi @haesleinhuepf

I intended to make the new parameters optional. Is that possible? Here is what I did

First the executeCL function checks the number of variables and sets the optional parameters.

https://github.com/clij/clij2-fft/blob/master/src/main/java/net/haesleinhuepf/clijx/plugins/DeconvolveRichardsonLucyFFT.java#L54

Second there are multiple DeconvolveRIchardsonLucyFFT functions. The original signature is still there

https://github.com/clij/clij2-fft/blob/master/src/main/java/net/haesleinhuepf/clijx/plugins/DeconvolveRichardsonLucyFFT.java#L75

So I had thought that all that changes is the GUI. However am I wrong on that and is the Macro calling different now?

haesleinhuepf commented 3 years ago

I intended to make the new parameters optional.

I tried to make macro parameters optional, but never managed to do that.

The original signature is still there

Yes, tha Java API signature is fine. I'm worried about the macro signature. But we can just derive another class from this one which only has a different getParameterHelpText function. Then we have two operations with different signatures based on the same code...

bnorthan commented 3 years ago

OK. Sounds like the simplest solutution is to add a 'Richardson Lucy Total Variation' plugin with the extra parameters, I'll try this and get back to you.

bnorthan commented 3 years ago

Hi Robert

See the last 2 commits https://github.com/clij/clij2-fft/commits/master

Do these address the issue?

haesleinhuepf commented 3 years ago

I just fixed a little typo and am optimistic that it would work. I guess re-compilation of the cpp part is not necessary to test this, right? I could give it a more detail try by the of the week. I could also compile cpp on Mac by then. Linux and Windows binaries are ready, right @bnorthan ?

bnorthan commented 3 years ago

Hi @haesleinhuepf

I recompiled the linux and windows libraries already. So all should be good with those.

haesleinhuepf commented 3 years ago

Hi Brian @bnorthan ,

ok, I compiled on MacOS, uploaded all binaries to the update site and tested the example macro on Mac and Windows successfully. See also #13 . After you merge it, we can officially release 2.2.0.15 .

Also, would you mind updating your Fiji on Linux and testing that it works?

Thanks!

Cheers, Robert

chalkie666 commented 3 years ago

@bnorthan @haesleinhuepf Hi guys. I'm back to looking at this again after my holiday.

2 questions:

1. I updated my Fiji today and got new clij2 components. Are the fixes Brian made for padding images as described above already included in the latest release? Thanks for those Brian!

2. My 2GB gpu runs out of memory, when it really should nkt get close to full using the modest sized images I am testing my imagej macro with. Raw image 31 megabytes Psf 10 megabytes. In my iterative algorithm I have about 8 or 9 copies of the raw image and 2 of the PSF image, as I process through the algorithm maths (I can't do inplace processing from imagej macro right?) The clij2 memory usage function tells me I'm using about 250 megabytes of my total 2 gigabytes at the end of each iteration. Should be fine.... But the operating system GPU memory usage monitor and the clij2 experimental graphical memory monitor both show my gpu memory getting filled up as the first couple of iterations run through. It looks like gpu memory is not released,, or reused. Maybe new copies of my gpu image variables are made at each iteration instead of being reused.???

Can you point me in the right direction? No doubt I'm doing something silly.

The macro is at the location on github I posted above

https://github.com/chalkie666/imagejMacros/blob/master/DeconvolutionDemos/TestCLIJ2FFTconvolve.ijm

Thanks for the fixes done recently, it's really helping me!!!

bnorthan commented 3 years ago

Hi @haesleinhuepf

Some how I missed your post. Sorry about that. Everything looks good... the only problem is that I had a major hard drive failure on my Linux Machine and it is unusable right now, so I can't run the Linux test.

I need to get up and going in Linux again anyway, so let me try to figure out the fastest way to get another Linux partition or Cloud instance going, I've been a bit lazy about this, because I want to figure out the best way to back up the OS (like with "time machine" or something), I had the data and code backed up, just not the OS.

Brian

chalkie666 commented 3 years ago

@bnorthan @haesleinhuepf Hi folks. Should I close this issue? Because Brian's image and psf padding fixes are released? I tested it quickly and it seems to work as expected now. Bi I should test with a range if image and psf sizes. I will do that as I continue.

I can ask my "looks like a memory leak, but I'm probably doing it wrong" question on the forum since it doesn't fit with this issue here.

bnorthan commented 3 years ago

Hi @chalkie666

Any luck in tracking down the memory leak? A while ago, and I asked @haesleinhuepf and others on the forum if there was a convenient helper function that would print out all allocated GPU buffers to help with trouble shooting. My understanding (correct me if I am wrong, and I am ignorant about a diagnostic memory function) is that thus far there is no such convenience method, and there are some technical reasons that it is not trivial to write one.

So you may have to do some "old school" trouble shooting. Have you tried commenting out parts of the loop to try to deduce which line is causing the memory leak? I know the deconvolution itself doesn't leak (I don't think)... I've run it hundreds of iterations. Could be the convolve, but theoretically it should behave the same as the deconvolution. I'll double check later this week.

Brian

chalkie666 commented 3 years ago

Hi @bnorthan @chalkie666 I'm not sure if it's a classic memory leak or I I'm just doing something silly.

I noticed I am decaring the variables for image copy I make in the algorithm, but the docs say I don't need to do that....

I'm only using the convolve function, plus some image math also on the gpu So leaks could be there. Because this is an interactive algorithm, I'm reusing image variables each iteration... Maybe im supposed to clear them explicitly each iteration, so new copies are made , even if the have the same name. ????

The clij2 diagnostics gadget that prints out the image names and sizes currently in gpu memory does NOT suggest I'm getting multiple copies building up every iteration, but the graphical gpu memory monitor DOES tell me gpu memory is getting eaten up.

Indeed I can try to isolate the command that is seemingly memory leaking... Is there a clever way to do that?

Best

D

haesleinhuepf commented 3 years ago

Hey Dan @chalkie666 ,

can you provide an example script and/or screenshot so that we can see what's happening?

Thanks!

chalkie666 commented 3 years ago

Hi @bnorthan @haesleinhuepf Seems like the convolution using FFT is the source of my issue. Here is a IJ macro that uses reasonable sized raw image and PSF ( 16 and and 10.5 Mb respectively.) https://github.com/chalkie666/imagejMacros/blob/master/DeconvolutionDemos/IterativeConvtestClij2fft.ijm Those 2 test images are in the same GIT repo. The for loop just redoes the same operation over and over, and input data and same expected output each iteration.

the GPU monitor from clij and the operating system both show the GPU memory filling up, but the Ext.CLIJ2_reportMemory()); function shows a constant 70 ish MB GPU memory used for 3 images, as the iterations roll through. With My 2GB GPU, it pukes after 2 iterations and the 3rd to 10 iterations whiz through generating images full of zeros. The IJ console says: CLIJ Error: Creating an image or kernel failed because your device ran out of memory.

Screen shot attached.

ConvFFT_potentialMemLeak

haesleinhuepf commented 3 years ago

Ok, I can just speculate. I see a minor memory leak here: normalization_factor is not released. @bnorthan , can you check if there are allocated images in the native code that might not be cleaned up?

bnorthan commented 3 years ago

Hi @chalkie666

I also found a second memory leak in the convolve. When you get a chance try this version in which the memory leak should be corrected. https://www.dropbox.com/sh/5g13o5f0c3azyh4/AACCZ7JVKyaiqlQ7teoC3Xqpa?dl=0

chalkie666 commented 3 years ago

Hi @bnorthan @haesleinhuepf Those memory leak fixes work for me. now memory use is stable over the iterations of the convolve test script, and of my algorithm prototype script. :-) thanks Guys! Now I can work on the fine tuning and verification etc. I owe you a beer or 2. Cheers!