fix: Updated BridgeTower Image processor #32384

Sai-Suraj-27 · 2024-08-01T18:43:46Z

What does this PR do?

Updated BridgeTower Image processor.

Fixes # (issue)

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
Did you read the contributor guideline,
Pull Request section?
Was this discussed/approved via a Github issue or the forum? Please add a link
to it if that's the case.
Did you make sure to update the documentation with your changes? Here are the
documentation guidelines, and
here are tips on formatting docstrings.
Did you write any new necessary tests?

Who can review?

@amyeroberts @ArthurZucker

amyeroberts

Thanks!

amyeroberts · 2024-08-01T18:55:48Z

@Sai-Suraj-27 This affects the image processing tests for bridgetower, which will need to be updated to reflect this fix

…to fix_assignment1

Sai-Suraj-27 · 2024-08-03T11:57:42Z

@Sai-Suraj-27 This affects the image processing tests for bridgetower, which will need to be updated to reflect this fix

Thanks for the review. Yes, I am looking into it. Will try to fix and update the PR 👍🏻
Edit: I updated the tests and now they are all passing. But I am not 100% sure, if I did it the correct way. Let me know if there is any problem.

…to fix_assignment1

amyeroberts · 2024-08-05T16:02:23Z

Hi @Sai-Suraj-27, thanks for iterating on this!

Looking at the changes in the test, I realized that this fix (which is addressing a bug in the code) would be introducing a change in the default behaviour of the image processor, and therefore would produce different outputs for the user. Previously, because do_center_crop was never defaulting to self.do_center_crop if unset, we were never cropping the image, and the flag in the preprocessor config had no effect.

What we should do is:

remove the do_center_crop flag from the image processor
remove crop_size logic from the image processor
Add a deprecation warning in the crop method to say that it's going to be removed in two releases time

Sai-Suraj-27 · 2024-08-06T05:55:53Z

Thanks for the review and explanation @amyeroberts. I've made the required changes. Please look into it and let me know if I missed anything.

…to fix_assignment1

amyeroberts

Thanks for tackling this! Just a few places we need to be careful to safely deprecate to avoid surprises for the user

amyeroberts · 2024-08-05T13:33:13Z

tests/models/bridgetower/test_image_processing_bridgetower.py

@@ -119,6 +122,8 @@ def get_expected_values(self, image_inputs, batched=False):
        return expected_height, expected_width

    def expected_output_image_shape(self, images):
+        if self.do_center_crop:
+            return self.num_channels, self.crop_size["height"], self.crop_size["width"]


To reflect the crop behaviour of the image processor

Suggested change

return self.num_channels, self.crop_size["height"], self.crop_size["width"]

return self.num_channels, self.size["shortest_edge"], self.size["shortest_edge"]

amyeroberts · 2024-08-05T13:33:19Z

tests/models/bridgetower/test_image_processing_bridgetower.py

@@ -58,6 +59,7 @@ def __init__(
        self.rescale_factor = rescale_factor
        self.do_normalize = do_normalize
        self.do_center_crop = do_center_crop
+        self.crop_size = crop_size if crop_size is not None else {"height": 288, "width": 288}


Suggested change

self.crop_size = crop_size if crop_size is not None else {"height": 288, "width": 288}

amyeroberts · 2024-08-05T13:33:24Z

tests/models/bridgetower/test_image_processing_bridgetower.py

@@ -73,6 +75,7 @@ def prepare_image_processor_dict(self):
            "do_normalize": self.do_normalize,
            "do_resize": self.do_resize,
            "size": self.size,
+            "crop_size": self.crop_size,


Suggested change

"crop_size": self.crop_size,

amyeroberts · 2024-08-05T13:36:38Z

src/transformers/models/bridgetower/image_processing_bridgetower.py

@@ -296,10 +296,9 @@ def center_crop(
                The channel dimension format of the input image. If not provided, it will be inferred from the input
                image.
        """
-        output_size = size["shortest_edge"]


In this case, what we want to do is update the docstring to reflect the true behaviour of the method, but still take the same size["shortest_edge"] argument. Otherwise this would be a breaking change and previous configs for the model would have a different behaviour. In fact - I think existing configs would break as they don't have "height" or "width" keys for size

amyeroberts · 2024-08-05T13:36:46Z

tests/models/bridgetower/test_image_processing_bridgetower.py

@@ -42,6 +42,7 @@ def __init__(
        rescale_factor: Union[int, float] = 1 / 255,
        do_normalize: bool = True,
        do_center_crop: bool = True,
+        crop_size: Dict[str, int] = None,


Suggested change

crop_size: Dict[str, int] = None,

amyeroberts · 2024-08-16T13:09:38Z

src/transformers/models/bridgetower/image_processing_bridgetower.py

@@ -277,6 +268,7 @@ def center_crop(
                The channel dimension format of the input image. If not provided, it will be inferred from the input
                image.
        """
+        warnings.warn("The center_crop method is deprecated and will be removed in two releases time.")


We should specify the exact release version.

amyeroberts · 2024-09-05T19:00:22Z

src/transformers/models/bridgetower/image_processing_bridgetower.py

-        do_center_crop: Optional[bool] = None,
-        crop_size: Dict[str, int] = None,


We can't just remove these like this as this will break things for users. We need to still accept them and emit a warning if they are set

amyeroberts · 2024-09-05T19:00:43Z

src/transformers/models/paligemma/modeling_paligemma.py

@@ -126,7 +126,6 @@ class PaliGemmaPreTrainedModel(PreTrainedModel):
    _no_split_modules = ["PaliGemmaMultiModalProjector"]
    _skip_keys_device_placement = "past_key_values"
    _supports_flash_attn_2 = False
-    _supports_cache_class = True


We shouldn't remove this here

…nto fix_assignment1

Rocketknight1 · 2025-01-13T17:29:42Z

cc @qubvel can you take a look at this while Amy is out?

ArthurZucker

Thanks for the PR, btw I am not sure I understand from reading the interactions what this fixes? It's also kinda breaking as you remove something but more importantly could you explain the motivations?

Sai-Suraj-27 · 2025-01-22T17:29:31Z

Thanks for the PR, btw I am not sure I understand from reading the interactions what this fixes? It's also kinda breaking as you remove something but more importantly could you explain the motivations?

Thanks for looking into the PR. Line 458, here is the initial reason I made the PR.

transformers/src/transformers/models/bridgetower/image_processing_bridgetower.py

Lines 455 to 458 in 2c3a44f

    
           image_mean = image_mean if image_mean is not None else self.image_mean 
        
           image_std = image_std if image_std is not None else self.image_std 
        
           do_pad = do_pad if do_pad is not None else self.do_pad 
        
           do_center_crop if do_center_crop is not None else self.do_center_crop

Initially i just changed the line to:

do_center_crop = do_center_crop if do_center_crop is not None else self.do_center_crop

But tht affected the image processing tests for bridgetower, which lead to these changes.

Fixed an incorrect variable assignment.

f435e3f

amyeroberts approved these changes Aug 1, 2024

View reviewed changes

Merge branch 'main' of https://github.com/huggingface/transformers in…

9760965

…to fix_assignment1

Sai-Suraj-27 added 3 commits August 4, 2024 21:58

Merge branch 'main' of https://github.com/huggingface/transformers in…

f7af440

…to fix_assignment1

Fixed failing tests.

973a8e1

Small nit.

e88f9ef

Sai-Suraj-27 changed the title ~~fix: Fixed an incorrect variable assignment~~ fix: Updated image_processing_bridgetower tests Aug 5, 2024

Sai-Suraj-27 added 3 commits August 6, 2024 11:16

Updated bridgetower image processor.

3213697

Updated bridgetower image processor.

16cf86d

Updated the warning message.

572a0ac

Sai-Suraj-27 changed the title ~~fix: Updated image_processing_bridgetower tests~~ fix: Updated bridgetower Image processor Aug 6, 2024

Sai-Suraj-27 changed the title ~~fix: Updated bridgetower Image processor~~ fix: Updated BridgeTower Image processor Aug 6, 2024

Sai-Suraj-27 added 2 commits August 6, 2024 16:59

Merge branch 'main' of https://github.com/huggingface/transformers in…

96b85ca

…to fix_assignment1

Merge branch 'main' of https://github.com/huggingface/transformers in…

87b0151

…to fix_assignment1

Sai-Suraj-27 requested a review from amyeroberts August 12, 2024 18:41

Sai-Suraj-27 added 2 commits August 14, 2024 23:45

Merge branch 'main' of https://github.com/huggingface/transformers in…

25a2058

…to fix_assignment1

Removed duplicate class attribute.

a2be255

amyeroberts reviewed Sep 5, 2024

View reviewed changes

Sai-Suraj-27 added 2 commits January 12, 2025 13:45

Updated bridgetower image processor.

0866470

Merge branch 'main' of github.com-personal:huggingface/transformers i…

57235cd

…nto fix_assignment1

Sai-Suraj-27 requested review from qubvel, Rocketknight1 and ArthurZucker as code owners January 12, 2025 13:46

ArthurZucker reviewed Jan 21, 2025

View reviewed changes

Merge branch 'main' into fix_assignment1

7a66f15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Updated BridgeTower Image processor #32384

fix: Updated BridgeTower Image processor #32384

Sai-Suraj-27 commented Aug 1, 2024 •

edited

Loading

amyeroberts left a comment

amyeroberts commented Aug 1, 2024

Sai-Suraj-27 commented Aug 3, 2024 •

edited

Loading

amyeroberts commented Aug 5, 2024

Sai-Suraj-27 commented Aug 6, 2024 •

edited

Loading

amyeroberts left a comment

amyeroberts Aug 5, 2024

amyeroberts Aug 5, 2024

amyeroberts Aug 5, 2024

amyeroberts Aug 5, 2024

amyeroberts Aug 5, 2024

amyeroberts Aug 16, 2024

amyeroberts Sep 5, 2024

amyeroberts Sep 5, 2024

Rocketknight1 commented Jan 13, 2025

ArthurZucker left a comment

Sai-Suraj-27 commented Jan 22, 2025 •

edited

Loading

	return self.num_channels, self.crop_size["height"], self.crop_size["width"]
	return self.num_channels, self.size["shortest_edge"], self.size["shortest_edge"]

		do_center_crop: Optional[bool] = None,
		crop_size: Dict[str, int] = None,

fix: Updated BridgeTower Image processor #32384

Are you sure you want to change the base?

fix: Updated BridgeTower Image processor #32384

Conversation

Sai-Suraj-27 commented Aug 1, 2024 • edited Loading

What does this PR do?

Before submitting

Who can review?

amyeroberts left a comment

Choose a reason for hiding this comment

amyeroberts commented Aug 1, 2024

Sai-Suraj-27 commented Aug 3, 2024 • edited Loading

amyeroberts commented Aug 5, 2024

Sai-Suraj-27 commented Aug 6, 2024 • edited Loading

amyeroberts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rocketknight1 commented Jan 13, 2025

ArthurZucker left a comment

Choose a reason for hiding this comment

Sai-Suraj-27 commented Jan 22, 2025 • edited Loading

Sai-Suraj-27 commented Aug 1, 2024 •

edited

Loading

Sai-Suraj-27 commented Aug 3, 2024 •

edited

Loading

Sai-Suraj-27 commented Aug 6, 2024 •

edited

Loading

Sai-Suraj-27 commented Jan 22, 2025 •

edited

Loading