JuliaMath / NFFT.jl

Julia implementation of the Non-equidistant Fast Fourier Transform (NFFT)
Other
153 stars 28 forks source link

Use mod instead of rem to prevent 0 or negative index. #141

Closed nbwuzhe closed 3 months ago

nbwuzhe commented 3 months ago

In some extreme cases, rem could return a negative number, giving a negative or zero value for idx_{d+1}. So mod should be used to make sure idx_{d+1} is positive to avoid any indexing error.

tknopp commented 3 months ago

Hi, could you be more concrete, in which cases that could happen? And have you experienced it in practice?

negative numbers should not happen since we add N to the left argument. 0 is ok, since we add 1 afterwards.

nbwuzhe commented 3 months ago

Hi, could you be more concrete, in which cases that could happen? And have you experienced it in practice?

negative numbers should not happen since we add N to the left argument. 0 is ok, since we add 1 afterwards.

Dears @tknopp

We encountered a memory issue that led to a random crashing of REPL for our SMS spiral reconstruction project (I added the 3rd dimension to FieldmapNFFTOp). This issue has been for quite a while, and since its random occurrence, it was a bit tricky to debug. Recently I noticed that many calculations in this package were run under @inbounds, which skips boundary checks to enhance performance. After enforcing the boundary check, we found the following values led to a zero idx_{d+1} for d = 2:

[ Info: idx_{d+1} = 0 for d = 2: l_{d+1} = 1, off[d+1] = -4, p.Ñ[d+1] = 2

I'm not sure if a negative offset with an absolute value larger than p.Ñ is normal (the "extreme cases" I mentioned), but using mod will avoid this issue. Please kindly advise.

Best regards,

Tim Zhe Wu

tknopp commented 3 months ago

Ok, thanks. The reason for using rem instead of mod was that it is faster. So the solution to the problem would actually be too understand the extreme case and fix it somehow. But the specific location where this currently happens is not the hottest loop so that it might be ok to use mod as a workaround.