Aggregator smaller model outputs - Ready to be reviewed #1002

wahabk · 2022-11-20T13:10:59Z

This is to reference #{1001}.

I have described the issue and planned fix in detail I believe, this is a draft PR to ask for some help

I have made most changes I believe, but this is a broken draft to show you my idea, let me know if the assertions are insufficient or if argument/attribute names are not clear

The bit I'm confused with is the subject padding, the subject is now forced to be padded and the padding is set to be at least the difference between input and output. and the final aggregator output tensor isn't padded

So say I have an input tensor with shape (128.128.128) , with a patch_size of (64,64,64) and a model output size of (60,60,60)

The patch_difference is set to be (4,4,4) and overlap is made to be at least equal to this, when the subject is padded in the GridSampler does that mean it's shape is now (136,136,136)?

Description
#1001

Checklist

I have read the CONTRIBUTING docs and have a developer setup (especially important are pre-commitand pytest)
Non-breaking change (would not break existing functionality)
Breaking change (would cause existing functionality to change)
Tests added or modified to cover the changes
Integration tests passed locally by running pytest
In-line docstrings updated
Documentation updated, tested running make html inside the docs/ folder
This pull request is ready to be reviewed
If the PR is ready and there are multiple commits, I have squashed them and force-pushed

check patch_size v patch_size in init hann check _crop_patch for Volume_padded and if overlap is bigger than patch_diff

codecov · 2022-11-27T14:12:04Z

Codecov Report

Merging #1002 (df0eca3) into main (aa4d609) will increase coverage by 0.03%.
The diff coverage is 89.65%.

@@            Coverage Diff             @@
##             main    #1002      +/-   ##
==========================================
+ Coverage   86.47%   86.51%   +0.03%     
==========================================
  Files          91       91              
  Lines        5774     5798      +24     
==========================================
+ Hits         4993     5016      +23     
- Misses        781      782       +1

Impacted Files	Coverage Δ
src/torchio/data/sampler/grid.py	`97.77% <85.71%> (-2.23%)`	⬇️
src/torchio/data/inference/aggregator.py	`96.12% <93.33%> (-0.52%)`	⬇️
...hio/transforms/augmentation/spatial/random_flip.py	`100.00% <0.00%> (+2.85%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

to depend on patch_diffs and model_output_size Expanded test_inference for different overlap modes Tests now working except for hann

Fixed for average but other crop and hann broken check patch_size v patch_size in init hann check _crop_patch for Volume_padded and if overlap is bigger than patch_diff changed crop_patch and hann window to depend on patch_diffs and model_output_size Expanded test_inference for different overlap modes Tests now working except for hann add docstring

…ahabk/torchio into 1001-aggregator-smaller-output

wahabk · 2022-11-29T15:53:27Z

This fixes #1001.

I have described the issue and in detail I believe #1001, I think this is PR is ready to be merged except for the final questions. I'm still getting my feet wet with PRs so would appreciate some guidance.

TLDR for why this is important:

I believe tio grid sampling and aggregating should be able to handle smaller model outputs. My model predictions are terrible even with averaging or hann windowing. Unfortunately most popular model libraries (such as the great monai) only provide models with the same output size and input. But it is crucial in my application to let the model see a bigger input ROI than semantic label outputs - by padding convolutions, as this gives context for the prediction. The original unet paper uses padded convolutions for smaller outputs than inputs.

Description

Summary of changes:

Add argument to GridSampler called model_output_size in case it is smaller than patch_size
This then computes patch_diffs which is the difference between them
GridSampler now makes sure that overlap_size is at least the same as patch_diffs
The locations are changed to fit the smaller patch_size
If overlap_mode is 'crop' it changes GridAggregator._crop_patch to only crop half_overlap - patch_diffs
If overlap_mode is 'hann' it makes the hann window of shape model_output_size instead of patch_size

Final questions before you agree:

The reason this bug was hidden from me is because the assertion in GridAggregator.add_batch checks target_spatial_shape from the provided batch, and not from self.patch_size. I belive this should be changed?
Does the kwarg name model_output_size make sense?
Are my new assertions thorough enough? Particularly in GridSampler.__init__() and GridAggregator.add_batch()
I have added a for loop in test_inference.py and test_inference_smaller.py that checks for different overlap_modes, however, the default tests fail under overlap mode 'hann'

Checklist

I have read the CONTRIBUTING docs and have a developer setup (especially important are pre-commitand pytest)
Non-breaking change (would not break existing functionality)
Breaking change (would cause existing functionality to change)
Tests added or modified to cover the changes
Integration tests passed locally by running pytest
In-line docstrings updated
Documentation updated, tested running make html inside the docs/ folder
This pull request is ready to be reviewed
If the PR is ready and there are multiple commits, I have squashed them and force-pushed

wahabk · 2022-11-29T15:56:13Z

@fepegar This is ready now when you have time to review. Please read the second comment, the first was the draft PR.

I tried squashing my commits using the tutorial, did it work correctly?

Any edits or requests you have in mind let me know and I will edit the PR.

fepegar · 2023-02-11T19:25:55Z

Hi, @wahabk. Thanks for the PR. Apologies for the delay. I will take a look at all this as soon as possible.

fepegar · 2023-03-04T13:50:11Z

tests/data/inference/test_inference_smaller.py

+from ...utils import TorchioTestCase
+
+
+class TestInference(TorchioTestCase):


Maybe we can add a test in the other file instead of creating this file which looks very similar.

fepegar · 2023-03-04T13:50:45Z

tests/data/inference/test_inference.py

+        for overlap_mode in [
+            'average',
+            'crop',
+        ]:  # not checking for 'hann' since assertiion fails


Do we know why the assertion fails?

fepegar · 2023-03-04T13:52:28Z

src/torchio/data/sampler/grid.py

+                self.padding_mode = 'constant'
+            self.patch_diffs = (
+                np.array(patch_size) - np.array(self.model_output_size)
+            ) // 2


What happens if the differences are not divisible by 2?

fepegar · 2023-03-04T17:03:06Z

src/torchio/data/inference/aggregator.py

+        # but remove input and output patch_diffs
+        # If patch_overlap is not bigger than patch_diffs.
+        # No need for cropping if output size is smaller
+        crop_ini = half_overlap.copy() - patch_diffs


Shouldn't this add instead of substract? Maybe I've misunderstood.

fepegar · 2023-03-04T17:04:13Z

src/torchio/data/inference/aggregator.py

-        target_spatial_shape = tuple(patch_sizes[0])
-        if input_spatial_shape != target_spatial_shape:
+        # Should target size be self.patch_size?
+        target_spatial_shape = tuple(


Why is it a tuple with only one element?

wahabk added 2 commits November 20, 2022 13:01

draft edits to show where to make changes

a891b13

Fixed for average but other crop and hann broken

3350706

check patch_size v patch_size in init hann check _crop_patch for Volume_padded and if overlap is bigger than patch_diff

wahabk added 4 commits November 29, 2022 14:54

changed crop_patch and hann window

7b67b85

to depend on patch_diffs and model_output_size Expanded test_inference for different overlap modes Tests now working except for hann

add docstring

75b0c9d

Merge branch '1001-aggregator-smaller-output' of https://github.com/w…

73fd6c6

…ahabk/torchio into 1001-aggregator-smaller-output

wahabk marked this pull request as ready for review November 29, 2022 15:53

wahabk changed the title ~~Aggregator smaller model outputs - draft edits to show where to make changes~~ Aggregator smaller model outputs - Ready to be reviewed Dec 14, 2022

Merge branch 'main' into 1001-aggregator-smaller-output

3c9853e

fepegar reviewed Mar 4, 2023

View reviewed changes

Merge branch 'main' into 1001-aggregator-smaller-output

df0eca3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aggregator smaller model outputs - Ready to be reviewed #1002

Aggregator smaller model outputs - Ready to be reviewed #1002

wahabk commented Nov 20, 2022 •

edited

codecov bot commented Nov 27, 2022 •

edited

wahabk commented Nov 29, 2022 •

edited

wahabk commented Nov 29, 2022

fepegar commented Feb 11, 2023

fepegar Mar 4, 2023

fepegar Mar 4, 2023

fepegar Mar 4, 2023

fepegar Mar 4, 2023

fepegar Mar 4, 2023

		from ...utils import TorchioTestCase


		class TestInference(TorchioTestCase):

Aggregator smaller model outputs - Ready to be reviewed #1002

Are you sure you want to change the base?

Aggregator smaller model outputs - Ready to be reviewed #1002

Conversation

wahabk commented Nov 20, 2022 • edited

codecov bot commented Nov 27, 2022 • edited

Codecov Report

wahabk commented Nov 29, 2022 • edited

wahabk commented Nov 29, 2022

fepegar commented Feb 11, 2023

fepegar Mar 4, 2023

Choose a reason for hiding this comment

fepegar Mar 4, 2023

Choose a reason for hiding this comment

fepegar Mar 4, 2023

Choose a reason for hiding this comment

fepegar Mar 4, 2023

Choose a reason for hiding this comment

fepegar Mar 4, 2023

Choose a reason for hiding this comment

wahabk commented Nov 20, 2022 •

edited

codecov bot commented Nov 27, 2022 •

edited

wahabk commented Nov 29, 2022 •

edited