Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

nbwuzhe
Copy link
Contributor

@nbwuzhe nbwuzhe commented Jul 19, 2024

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
Copy link
Member

tknopp commented Jul 19, 2024

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
Copy link
Contributor Author

nbwuzhe commented Jul 19, 2024

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
Copy link
Member

tknopp commented Jul 23, 2024

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.

@tknopp tknopp merged commit 4c9dbce into JuliaMath:master Jul 23, 2024
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants