Fix for remove_outlier3d #622 #623
Conversation
|
Hello @dkazanc! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2023-09-28 15:42:00 UTC |
source/libtomo/misc/median_filt3d.c
Outdated
| @@ -98,7 +100,8 @@ medfilt3D_float(float* Input, float* Output, int radius, int sizefilter_total, | |||
| k1 = k + k_m; | |||
| if((k1 < 0) || (k1 >= dimZ)) | |||
| k1 = k; | |||
| ValVec[counter] = Input[(dimX * dimY) * k1 + j1 * dimX + i1]; | |||
| index1 = (size_t)(dimX * dimY * k1 ) + (size_t)(j1 * dimX + i1); | |||
There was a problem hiding this comment.
What's the pro/con of using size_t for all indexing and size variables instead of a mix of long and size_t with casting? Do some of these indexing variable need to represent negative integers?
My concern with mixing types and casting on lines like this is that it might still overflow before the cast to size_t? Because the order of operations is to perform the computation inside the parenthesis before casting.
Maybe it should be something like this? Where everything that is not cast is already size_t? This way the multiplications are computed using types that are already size_t.
| index1 = (size_t)(dimX * dimY * k1 ) + (size_t)(j1 * dimX + i1); | |
| index1 = dimX * dimY * (size_t)k1 + (size_t)j1 * dimX + (size_t)i1; |
There was a problem hiding this comment.
thanks, makes total sense. I guess (size_t)(dimX * dimY * k1 ) could overflow before getting converted to size_t. will try to fix...
|
Thanks for starting a fix for this @dkazanc! I was waiting for confirmation that the problem was reproducable with arbitrary arrays. Questions/comments above. |
|
@carterbox are you OK with the corrections? thx |
We want to use size_t as much as possible when computing sizes and linear offsets, but we don't want to break backward compatability with the ABI.
Faster solves prevent CI from running out of time CI: Just add solver argument Remove dead code
The main issue was around
memcpyseg faulting, possibly when the total index size overflowsint.But in fact, there were multiple issues:
size_tdata type, which should work well across different OS.np.ascontiguousarray(arr), outbut should benp.ascontiguousarray(arr), np.ascontiguousarray(out).The combination of above seemed solved the problem.
Fixes #622
Closes #624