timothygrant80 / cisTEM

Other
32 stars 27 forks source link

Fixes (Gpu)Image::SwapRealSpaceQuadrants() for odd sized images. #465

Closed bHimes closed 1 year ago

bHimes commented 1 year ago

Fixes (Gpu)Image::SwapRealSpaceQuadrants() for odd sized images.

Fixes GpuImage::ReturnSumOfSquares, which was calling BufferInit before NPPInit, leaving the NPP buffer allocation based on uninitialized values.

Adds Debug asserts to catch the above scenario.

Adds testing for even/odd 2d/3d images for SwapQuadrants in console test. Only the Image class is tested, not GpuImage.

Adds include guards for gpu_image_cuFFT_callbacks.h.

Adds to git ignore some extensions for images and interactive runs (dff) when testing is done in the primary repo. This is convenient in the container where not all folders are mounted.

Description

For odd images, applying SwapRealSpaceQuadrants twice did not return the starting image. The origin was left at the "top right" (image below with value 1 placed at box center, then SwapQuadrants applied. Expected 1 in the lower left corner.) rather than wrapping around to the "bottom left" after 1 application. The error was compounded by the -1 on the return trip. The fix is to shift 1 extra pixel from centered -> corner, then shift by N/2 on the corner->centered.

This fix could create problems if there is another method that has a compensatory error. For example Image::FindPeakAtOriginFast2D()

SwapQuadrants is used in many places, for example, to make a cross-correlation come out with the peaks centered in the box. Most interesting, and deserving a look is in interpolation, where the origin of rotation is shifted to the corner prior to interpolating. I don't see how this could have worked properly before on odd sized images.

image

zoom in of an Overlay of unit impulse prior to shift, and after a round trip Swap, Swap

image

Fixes # (issue)

I have rebased my feature branch to be current with the master branch using to minimize conflicts and headaches

Which compilers were tested

These changes are isolated to the

How has the functionality been tested?

Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.

Checklist:

jojoelfe commented 1 year ago

The test makes sense to me, so things seem to be more correct now.

I get the fear that this introduces problems... I guess all we can do is pull it and test for a while if it behaves ok during stuff.

Also the X dimension will always be even, right, due to the fftw padding_factor? Does it make a difference whether X or Y are odd?

bHimes commented 1 year ago

The test makes sense to me, so things seem to be more correct now.

I get the fear that this introduces problems... I guess all we can do is pull it and test for a while if it behaves ok during stuff.

There are a couple more active things we could do: 1) Test actively comparing some common processing (tutorial data etc.) pre/post PR 2) (My favorite) just drop support for odd sized images. Force images and resampling to an even size, which will always perform better anyway.

Also the X dimension will always be even, right, due to the fftw padding_factor? Does it make a difference whether X or Y are odd?

No this isn't right. It is the logical dimension, not the physical dimension that matters in this case anyway.

bHimes commented 1 year ago

@jojoelfe I added new tests on SwapQuadrants that should catch any compensatory bugs in ExtractSlice(). It turns out that the method should have never produced an accurate projection for an odd sized 3d.

Here are some simple projections, rotated by 90 on z, cut out from a larger 3d:

While working out this test, I also realized that Image::ReturnSumOfSquares() has also never worked right for 3d FFTs, always underestimating the power in Fourier space. I extended the existing (CPU) tests as well as fixing the bug.