Skip to content

Fixed incorrect normalization #40436

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.

Sign up for GitHub

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

Merged
qubvel merged 1 commit into from
Aug 26, 2025
Merged

Fixed incorrect normalization #40436

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/transformers/image_processing_utils_fast.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,9 +304,9 @@ def compile_friendly_resize(
A wrapper around `F.resize` so that it is compatible with torch.compile when the image is a uint8 tensor.
"""
if image.dtype == torch.uint8:
image = image.float() / 256
image = image.float() / 255
image = F.resize(image, new_size, interpolation=interpolation, antialias=antialias)
image = image * 256
image = image * 255
image = torch.where(image > 255, 255, image)
image = torch.where(image < 0, 0, image)
Comment on lines 310 to 311
Copy link
Contributor

@qubvel qubvel Sep 2, 2025

Choose a reason for hiding this comment

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

btw, it might be more optimal to use torch.clamp(image, 0, 255) once instead of torch.where twice

Copy link
Collaborator

@remi-or remi-or Sep 2, 2025

Choose a reason for hiding this comment

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

Normally I would agree! But this is on purpose : #38540

Copy link
Contributor

@qubvel qubvel Sep 3, 2025

Choose a reason for hiding this comment

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

Ok, according to the PR it seems we have to revert this PR to use 256 and keep torch.where. To prevent this code from further regression we have to either add tests that fails on CI (cuda) if modified or properly comment the code

Copy link
Collaborator

@remi-or remi-or Sep 3, 2025

Choose a reason for hiding this comment

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

The regression this PR introduced already caused failures on the AMD CI, which is as important as NVIDIA (or cuda) CI!
As for properly commenting the code, both code paths where compile_friendly_resize is called are commented. You can check it out by expanding the diff, those lines are right above the function 🙂
If you want, we can add # this is to match torchvision.resize next to 256 and # We use torch.where instead of torch.clamp to avoid an error with torch.compile as comments to make sure no one will introduce the regression again. Wdyt?

Copy link
Contributor

@qubvel qubvel Sep 3, 2025

Choose a reason for hiding this comment

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

@remi-or absolutely agree AMD CI is as important as NVIDIA, what I'm trying to say is that we need a test that fails in PR's CI to prevent merging this PR. In terms of comments, yeah, it's better to comment non-obvious code right in place, otherwise it looks like a typo and is easy to miss the comment located in a different part (and that's happened in this case).
I'll do a quick fix for this, thanks for jumping in and clarifying 🤗

image = image.round().to(torch.uint8)
Expand Down