Skip to content

Don't hardcode 255 unless uint8 is enforced #6825

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

Closed
pmeier opened this issue Oct 24, 2022 · 3 comments · Fixed by #6826 or #6830
Closed

Don't hardcode 255 unless uint8 is enforced #6825

pmeier opened this issue Oct 24, 2022 · 3 comments · Fixed by #6826 or #6830

Comments

@pmeier
Copy link
Collaborator

pmeier commented Oct 24, 2022

Across our transformations we sometimes hardcode the value 255. This is justified if we make sure that point only torch.uint8 images are allowed, like

if interpolation == "bicubic" and out_dtype == torch.uint8:
img = img.clamp(min=0, max=255)

However, there are a few instances where uint8 is implied but never enforced:

bound = 1.0 if img1.is_floating_point() else 255.0
return (ratio * img1 + (1.0 - ratio) * img2).clamp(0, bound).to(img1.dtype)

bound = torch.tensor(1 if img.is_floating_point() else 255, dtype=img.dtype, device=img.device)
return bound - img

bound = 1.0 if img.is_floating_point() else 255.0

Instead of hardcoding 255 here, we should either use _max_value(dtype) instead or if uint8 is actually required, enforce it.

cc @vfdev-5 @datumbox

@datumbox
Copy link
Contributor

Good spot. It's also worth noting that the current stable uses incorrect bounds across methods. The ones that you highlighted use 255 for all integers. On the other hand, convert_image_dtype uses different bound depending on their integer type:

def _max_value(dtype: torch.dtype) -> int:
if dtype == torch.uint8:
return 255
elif dtype == torch.int8:
return 127
elif dtype == torch.int16:
return 32767
elif dtype == torch.int32:
return 2147483647
elif dtype == torch.int64:
return 9223372036854775807

These two need to align. Either we will continue assuming that 255 is the right value for integers, or as you propose we should use the _max_value(dtype) everywhere else.

@pmeier
Copy link
Collaborator Author

pmeier commented Oct 24, 2022

I prefer the behavior of convert_image_dtype for two reasons:

  1. If we would go for enforcing [0, 255] for image dtypes, it makes no sense to use to allow integer dtypes other than uint8 in the first place. While float16 or float64 share the same value range with the default float32, their precision varies and thus it makes sense for them to have the same range. However, there is no such effect for integer dtypes. They would have the exact same number of valid values at the cost of increased memory in case of int{16, 32, 64}.
  2. Changing the behavior of convert_image_dtype is BC breaking whereas changing the behavior of the ops mentioned above can be regarded as a bug fix. They were never doing the right thing unless one used uint8 or floating images.

@pmeier
Copy link
Collaborator Author

pmeier commented Oct 28, 2022

One more thing came to mind: although an edge case at best, by hardcoding 255 we we not only ignore large portions of the range for larger dtypes, we also don't handle the fact that torch.int8 images can only store [0, 127]. For example, with the current implementation F.posterize(image_int8, bits=4) is a no-op.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants