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

Structure splitting unsigned underflow #435

Closed
matham opened this issue Jun 3, 2024 · 2 comments · Fixed by #437 · May be fixed by #440
Closed

Structure splitting unsigned underflow #435

matham opened this issue Jun 3, 2024 · 2 comments · Fixed by #437 · May be fixed by #440
Labels
bug Something isn't working

Comments

@matham
Copy link
Contributor

matham commented Jun 3, 2024

Describe the bug
In iterative_ball_filter, we subtract 1 from the volume. Volume is an unsigned 32-bit array where bright areas are marked with the threshold value and everywhere else is zero. By subtracting 1, we underflow back to the max value, which is above the threshold, accidentally marking all the voxels as bright.

I'm testing with a very bright brain, which has a lot of noise but is great for testing parity between my pytroch changes and main. After getting parity in 2d/3d filtering, I couldn't get parity of results of detection between main and pytorch until I noticed this issue.

I fixed it temporarily by changing

vol -= 1 to vol[:, :, :] = np.where(vol != 0, vol - 1, 0). And that brought parity finally.

Here's the cell matching from main, to main with the change above. 96% of the cells were identical:

main_with_fix

I expect with clean quiet brains you wouldn't notice this much, but with noisy brains it should be pretty clear.

I'm not submitting it as a fix, since the pytorch branch should fix it. But I can make the fix (or if you want to do it), if you'd rather have main fixed meanwhile.


There's a secondary issue nearby in ball_filter_imgs.

for z in range(volume.shape[2]):
bf.append(volume[:, :, z].astype(np.uint32), good_tiles_mask[:, :, z])
if bf.ready:
bf.walk()
middle_plane = bf.get_middle_plane()
ball_filtered_volume[:, :, z] = middle_plane[:]

Called by this:

for i in range(n_iter):
vol, cell_centres = ball_filter_imgs(
vol, threshold_value, soma_centre_value
)

Due to 3d filtering, say with a kernel of size 3 in z. Then for 3 planes, the middle plane is what gets filtered - that's how volume_filter.py logic works and even in the line above where we set start_z=ball_z_size // 2.

However, bf.ready is True after we passed in 3 planes. Meaning the first z out as get_middle_plane is the values for the middle plane (plane 2). But, this gets assigned to plane 3 if you track z in the loop. Because this edited volume is returned in the function and passed in again, everything gets shifted up by one z plane, if you track what I'm describing.

Anyway, that will also be fixed in the pytorch branch.

@matham matham added the bug Something isn't working label Jun 3, 2024
@adamltyson
Copy link
Member

Thanks for raising this @matham.

I think it's best to have these fixes in the numba version too, because:

  1. I imagine we may need to fall back to the numba version sometimes (at least in the short term)
  2. It will help future debugging of the torch version if we know the numba version is correct (as far as we know)

What do you think @alessandrofelder?

@matham if you're able to submit a PR, that would be fantastic.

@alessandrofelder
Copy link
Member

Yep, I agree with you on this @adamltyson

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants