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

6193 add a buffer_step option to the sliding windows util/speed up #6254

Merged
merged 35 commits into from
Apr 5, 2023

Conversation

wyli
Copy link
Contributor

@wyli wyli commented Mar 29, 2023

Fixes #6193

Description

  • adds buffer_step (code adapted from @myron's idea/implementation)
  • clean up multioutput resizing logic
  • perf optimize
  • overlap to support different values along the spatial dimensions

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@wyli wyli changed the title add a buffer_step option to the sliding windows util/speed up 6193 add a buffer_step option to the sliding windows util/speed up Mar 29, 2023
@wyli wyli added this to the Auto3DSeg enhancement [P0 v1.2] milestone Mar 29, 2023
@myron myron mentioned this pull request Mar 29, 2023
wyli added 4 commits March 30, 2023 16:19
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli wyli force-pushed the sliding-windows branch from 6a5e39a to 0c11323 Compare March 31, 2023 01:54
wyli added 5 commits March 31, 2023 06:18
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli wyli force-pushed the sliding-windows branch from 7593660 to 011854f Compare March 31, 2023 16:38
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli wyli force-pushed the sliding-windows branch from ffd4ecb to 9a36eee Compare March 31, 2023 16:52
wyli added 3 commits April 1, 2023 15:01
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli wyli marked this pull request as ready for review April 1, 2023 15:41
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli
Copy link
Contributor Author

wyli commented Apr 1, 2023

/build

Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli
Copy link
Contributor Author

wyli commented Apr 1, 2023

/build

Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli wyli force-pushed the sliding-windows branch from 581181f to 8cd28e9 Compare April 1, 2023 18:07
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli wyli force-pushed the sliding-windows branch from 3c05d17 to ad8c9f8 Compare April 2, 2023 15:00
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli wyli force-pushed the sliding-windows branch from 199d716 to 5d4e17a Compare April 2, 2023 15:15
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli wyli force-pushed the sliding-windows branch from 5d4e17a to d787eef Compare April 2, 2023 15:17
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@myron
Copy link
Collaborator

myron commented Apr 3, 2023

I'm going to test it

Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Copy link
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

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

Looks good to me.
SlidingWindow is quite important and widely used in MONAI applications, please ensure to test it in typical tutorials and bundles before merging.
@KumoLiu Please also help review it.

Thanks.

wyli and others added 3 commits April 3, 2023 11:11
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli
Copy link
Contributor Author

wyli commented Apr 4, 2023

/build

wyli and others added 8 commits April 5, 2023 11:45
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli
Copy link
Contributor Author

wyli commented Apr 5, 2023

/build

@wyli wyli enabled auto-merge (squash) April 5, 2023 22:02
@wyli wyli merged commit f98f0fd into Project-MONAI:dev Apr 5, 2023
@myron
Copy link
Collaborator

myron commented Apr 6, 2023

@wyli this looks great! thanks for writing.

a few comments

  1. perhaps the default for buffer_dim should be -1 (as this is the most common)
  2. SlidingWindowInferer parameter type for "overlap" should be updated to overlap: float | Sequence (since it accepts a list now)
  3. setting buffer_step option seems only appropriate together with setting device='cpu". It wasn't obvious. I tried using buffer_step=1, and GPU memory increased (compared to buffer_step==None). and it took me some time to figure out that device parameter needs to be manually specified to cpu. a user usually leaves that option as a default None, so it won't be obvious. may be we can give a warning, or just automatically set device to 'cpu' when the buffer_step is not None.

thanks!

@wyli
Copy link
Contributor Author

wyli commented Apr 6, 2023

thanks @myron, I'll revise it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Optimized sliding window inferer
3 participants