Fix tiling for Reverse Distillation and STFPM #1319

blaz-r · 2023-09-01T15:12:13Z

Description

This PR fixes the tiling for STFPM and Reverse Distillation models. It also adds tiling support to Fastflow.

For STFPM and Reverse Distillation, instead of relying on the tiler initialization for size, we avoid this issue by rather untiling the features right before they are returned or use in anomaly map generator.

The implementation of Fastflow tiling is up to debate, but I tried to keep the predictions in tiled shape for the longest duration of procedure to offer the best benefits of reduced memory usage.

This PR does NOT add the callback for tiling setup because it's currently not present so I avoided adding it. To utilize (and verify) tiling, the following should be added to the callbacks:

tiling = TilerConfigurationCallback(
    enable=config.dataset.tiling.apply,
    tile_size=config.dataset.tiling.tile_size,
    stride=config.dataset.tiling.stride,
)
callbacks.append(tiling)

Fixes [Bug]: Tiling is not working correctly for some models #1318

Changes

Describe the changes you made

Bug fix (non-breaking change which fixes an issue)
Refactor (non-breaking change which refactors the code base)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update

Checklist

Ensure that you followed the following

I have added a summary of my changes to the CHANGELOG (not for minor changes, docs and tests).
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas (If applicable)
I have made corresponding changes to the documentation (If applicable)
I have added tests that prove my fix is effective or that my feature works (If applicable)

samet-akcay · 2023-09-04T15:37:51Z

@blaz-r, the tests seem to fail. Will you be able to have a look?

blaz-r · 2023-09-04T16:21:51Z

Okay sure. I think I broke backwards compatibility for fastflow. I'll fix this and rerun tests locally.

blaz-r · 2023-09-04T21:13:58Z

Tests are now passing, and fastflow is now backwards compatible.
I see that coverage dropped, because tiling in models is not covered with tests. As far as I am aware, this is not covered for any model, so I didn't add tests, maybe I should?

samet-akcay · 2023-09-11T16:18:54Z

src/anomalib/models/fastflow/lightning_model.py

@@ -21,6 +21,7 @@ class Fastflow(AnomalyModule):

    Args:
        input_size (tuple[int, int]): Model input size.
+        image_size (tuple[int, int]): Original image size (needed in case of tiling).


Would having both image_size and input_size in the model definition be confusing to the user? @ashwinvaidya17, any thoughts?

Maybe we can just call it original_image_size?

Also, not sure if I quite understood it. The only place we use the original_image_size is in anomaly_map_generator so that it can interpolate the flow maps. But why can't we pass the model input_size? Aren't the tiles generated from the input tensor that is in shape of model input_size? Also, won't the visualizer take care of re-scaling it further? Am I missing something?

Input size is in case of tiling equal to tile size, because the fastflow blocks requires actual input size at the time of initialization. If we pass just tile size, we can't really know the size of anomaly maps, and if we pass just image size (by default also input size), we can't init the model as we don't know the tile size in the constructor.

This is not a problem for other models as they don't require tile size in their constructors.
Visualiser can handle this, but the metrics don't, as they expect the same size of GT and anomaly map.

On the other hand, I think this could be renamed to anomal_map_size, which would be better than image_size or original_image_size.

Alright thanks for the explination. Yeah we can rename it to anomaly_map_size. Apart from this I have no other suggestions.

samet-akcay · 2023-09-11T16:20:24Z

src/anomalib/models/reverse_distillation/torch_model.py

-            image_size = input_size
-
-        self.anomaly_map_generator = AnomalyMapGenerator(image_size=image_size, mode=anomaly_map_mode)
+        self.anomaly_map_generator = AnomalyMapGenerator(image_size=input_size, mode=anomaly_map_mode)


A reference for future, but not it might make more sense to rename image_size argument to input_size in AnomalyMapGenerator. As I said, it is not related to this PR, though

samet-akcay

@blaz-r, thanks for creating this PR. I have a single comment, which we could perhaps discuss with @ashwinvaidya17

blaz-r · 2023-09-11T17:55:18Z

@blaz-r, thanks for creating this PR. I have a single comment, which we could perhaps discuss with @ashwinvaidya17

Okay. Yeah the naming is a bit weird and I'm open to suggestions for renaming. I'm just not sure what to call image_size since it's called that in AnomalyMapGenerator. In case of normal FastFlow, input size is also image size, but when tiling is performed, input size is actually tile size and image size is image size, so I'm not sure how to name these so this is clearly indicated.

ashwinvaidya17

Except for the same input_size comment raised by Samet I have no other feedback. Just one question though

ashwinvaidya17 · 2023-09-12T08:12:15Z

src/anomalib/models/fastflow/lightning_model.py

@@ -21,6 +21,7 @@ class Fastflow(AnomalyModule):

    Args:
        input_size (tuple[int, int]): Model input size.
+        image_size (tuple[int, int]): Original image size (needed in case of tiling).


Maybe we can just call it original_image_size?

ashwinvaidya17 · 2023-09-12T08:38:55Z

src/anomalib/models/fastflow/lightning_model.py

@@ -21,6 +21,7 @@ class Fastflow(AnomalyModule):

    Args:
        input_size (tuple[int, int]): Model input size.
+        image_size (tuple[int, int]): Original image size (needed in case of tiling).


Also, not sure if I quite understood it. The only place we use the original_image_size is in anomaly_map_generator so that it can interpolate the flow maps. But why can't we pass the model input_size? Aren't the tiles generated from the input tensor that is in shape of model input_size? Also, won't the visualizer take care of re-scaling it further? Am I missing something?

blaz-r · 2023-09-16T08:51:12Z

image_size is now renamed to anomaly_map_size.

ashwinvaidya17

Thanks again for the fix

blaz-r · 2023-09-18T08:32:51Z

Fastflow tiling will be refactored to match other architectures.

blaz-r · 2023-09-21T18:18:23Z

The addition to fastflow is reverted now. This PR only fixes tiling for Reverse Distillation and STFPM.

samet-akcay

lgtm, thanks

blaz-r added 3 commits September 1, 2023 16:51

Fix tiling for stfpm

f32def8

Fix tiling for reverse distillation

3bfd213

Add tiling to fastflow

578f871

blaz-r requested review from samet-akcay and ashwinvaidya17 as code owners September 1, 2023 15:12

Merge branch 'main' into tiling_fix

1741a51

Make tiling in fastflow backwards compatible

3a5a4e8

blaz-r added 2 commits September 5, 2023 23:21

Merge branch 'openvinotoolkit:main' into tiling_fix

5f2777d

Merge branch 'main' into tiling_fix

0e3a791

samet-akcay reviewed Sep 11, 2023

View reviewed changes

ashwinvaidya17 reviewed Sep 12, 2023

View reviewed changes

blaz-r and others added 2 commits September 16, 2023 10:34

Merge branch 'openvinotoolkit:main' into tiling_fix

f41c137

Rename image_size to anomaly_map_size.

c9b6fc3

ashwinvaidya17 approved these changes Sep 18, 2023

View reviewed changes

blaz-r and others added 4 commits September 18, 2023 19:03

Merge branch 'openvinotoolkit:main' into tiling_fix

4d30ef6

Merge branch 'openvinotoolkit:main' into tiling_fix

9765c79

Reverse fastflow tiling

7981bf0

Remove tiling calls in fastflow

6180a39

blaz-r changed the title ~~Fix tiling and add it to Fastflow~~ Fix tiling for Reverse Distillation and STFPM Sep 21, 2023

samet-akcay approved these changes Sep 21, 2023

View reviewed changes

Merge branch 'main' into tiling_fix

c2ca150

samet-akcay merged commit 2a04129 into openvinotoolkit:main Sep 22, 2023
4 checks passed

blaz-r deleted the tiling_fix branch October 26, 2023 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tiling for Reverse Distillation and STFPM #1319

Fix tiling for Reverse Distillation and STFPM #1319

blaz-r commented Sep 1, 2023

samet-akcay commented Sep 4, 2023

blaz-r commented Sep 4, 2023

blaz-r commented Sep 4, 2023

samet-akcay Sep 11, 2023

ashwinvaidya17 Sep 12, 2023

ashwinvaidya17 Sep 12, 2023

blaz-r Sep 12, 2023 •

edited

Loading

ashwinvaidya17 Sep 13, 2023

samet-akcay Sep 11, 2023

samet-akcay left a comment

blaz-r commented Sep 11, 2023

ashwinvaidya17 left a comment

ashwinvaidya17 Sep 12, 2023

ashwinvaidya17 Sep 12, 2023

blaz-r commented Sep 16, 2023

ashwinvaidya17 left a comment

blaz-r commented Sep 18, 2023

blaz-r commented Sep 21, 2023

samet-akcay left a comment

Fix tiling for Reverse Distillation and STFPM #1319

Fix tiling for Reverse Distillation and STFPM #1319

Conversation

blaz-r commented Sep 1, 2023

Description

Changes

Checklist

samet-akcay commented Sep 4, 2023

blaz-r commented Sep 4, 2023

blaz-r commented Sep 4, 2023

samet-akcay Sep 11, 2023

Choose a reason for hiding this comment

ashwinvaidya17 Sep 12, 2023

Choose a reason for hiding this comment

ashwinvaidya17 Sep 12, 2023

Choose a reason for hiding this comment

blaz-r Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

ashwinvaidya17 Sep 13, 2023

Choose a reason for hiding this comment

samet-akcay Sep 11, 2023

Choose a reason for hiding this comment

samet-akcay left a comment

Choose a reason for hiding this comment

blaz-r commented Sep 11, 2023

ashwinvaidya17 left a comment

Choose a reason for hiding this comment

ashwinvaidya17 Sep 12, 2023

Choose a reason for hiding this comment

ashwinvaidya17 Sep 12, 2023

Choose a reason for hiding this comment

blaz-r commented Sep 16, 2023

ashwinvaidya17 left a comment

Choose a reason for hiding this comment

blaz-r commented Sep 18, 2023

blaz-r commented Sep 21, 2023

samet-akcay left a comment

Choose a reason for hiding this comment

blaz-r Sep 12, 2023 •

edited

Loading