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

Prevent zero underflow in volume #437

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

matham
Copy link
Contributor

@matham matham commented Jun 7, 2024

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

Currently volume underflows when we subtract 1, as explained in the bug report. Because volume is unsigned so subtracting from zero will flow back to the max value, that actually means they become bright voxels.

Also, the z calculation shifts the z stack as explained there.

What does this PR do?

Prevent the underflow by not subtracting from zero entries. Also correctly calculates the z pos of processed middle planes.

References

Fixes #435.

How has this PR been tested?

Added a test. Functionality probably has changed though because it was incorrect before.

Without the fix, the newly added test prints the following when test:

================================================================================================================== FAILURES ==================================================================================================================
__________________________________________________________________________________________________________ test_underflow_issue_435 __________________________________________________________________________________________________________

    def test_underflow_issue_435():
        # two cells centered at (9, 10, 10), (17, 10, 10) with radius 4
        p1 = np.array([9, 10, 10])
        p2 = np.array([19, 10, 10])
        radius = 5

        bright_voxels = np.zeros((20, 20, 30), dtype=np.bool_)

        pos = np.empty((20, 20, 30, 3))
        pos[:, :, :, 0] = np.arange(20).reshape((-1, 1, 1))
        pos[:, :, :, 1] = np.arange(20).reshape((1, -1, 1))
        pos[:, :, :, 2] = np.arange(30).reshape((1, 1, -1))

        dist1 = pos - p1.reshape((1, 1, 1, 3))
        dist1 = np.sqrt(np.sum(np.square(dist1), axis=3))
        inside1 = dist1 <= radius
        dist2 = pos - p2.reshape((1, 1, 1, 3))
        dist2 = np.sqrt(np.sum(np.square(dist2), axis=3))
        inside2 = dist2 <= radius

        bright_voxels[np.logical_or(inside1, inside2)] = True
        bright_indices = np.argwhere(bright_voxels)

        centers = split_cells(bright_indices)

        # for some reason, same with pytorch, it's shifted by 1. Probably rounding
        expected = {(10, 11, 11), (18, 11, 11)}
        got = set(map(tuple, centers.tolist()))
>       assert expected == got
E       assert {(10, 11, 11), (18, 11, 11)} == {(10.0, 11.0,..., 11.0, 12.0)}
E
E         Extra items in the left set:
E         (18, 11, 11)
E         (10, 11, 11)
E         Extra items in the right set:
E         (10.0, 11.0, 12.0)
E         (12.0, 11.0, 11.0)
E         (18.0, 11.0, 12.0)
E         Use -v to get more diff

tests\core\test_integration\test_detection_structure_splitting.py:81: AssertionError

========================================================================================================== short test summary info ===========================================================================================================
FAILED tests/core/test_integration/test_detection_structure_splitting.py::test_underflow_issue_435 - assert {(10, 11, 11), (18, 11, 11)} == {(10.0, 11.0,..., 11.0, 12.0)}
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
===================================================================================================== 1 failed, 99 deselected in 21.21s ======================================================================================================

Is this a breaking change?

Not in the API

Does this PR require an update to the documentation?

No.

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality (unit & integration)
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

Copy link
Member

@alessandrofelder alessandrofelder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little question about the test setup, otherwise this looks great! Thanks so much @matham!

@matham
Copy link
Contributor Author

matham commented Jun 10, 2024

For reference, here's the cell count comparison for our 1555x3222x3848 brain of main vs this branch.
main_vs_main_with_fix

@alessandrofelder alessandrofelder merged commit 352aa40 into brainglobe:main Jun 10, 2024
18 checks passed
@matham matham deleted the split_underflow branch June 10, 2024 12:32
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.

Structure splitting unsigned underflow
2 participants