Add torchscriptable adjust_gamma transform #2459
Conversation
| @@ -161,8 +161,8 @@ def convert_image_dtype(image: torch.Tensor, dtype: torch.dtype = torch.float) - | |||
| msg = f"The cast from {image.dtype} to {dtype} cannot be performed safely." | |||
| raise RuntimeError(msg) | |||
|
|
|||
| eps = 1e-3 | |||
| return image.mul(torch.iinfo(dtype).max + 1 - eps).to(dtype) | |||
There was a problem hiding this comment.
this was giving results that didn't match PIL
There was a problem hiding this comment.
Hum, we had quite a lot of discussion about this behavior in #2078 (comment). I believe if we make the multiplication go to dtype.max, we will end up with a non-uniform distribution over the last values.
@pmeier thoughts?
There was a problem hiding this comment.
@fmassa is right about the intention. I think this boils down to
- do we want it "right" or
- do we want it compatible to other packages.
I'm in favor for the former (hence my implementation), but I can see why the latter is also feasible.
There was a problem hiding this comment.
I think my issue was internal consistency, not a difference between us and PIL. That comment thread is helpful -- we certainly expect more than one floating point value to map to 255.
I was able to fix it by just making the gamma adjustment consistent with convert_image_dtype.
This raises another issue though -- I'm not sure if it's still OK to express the equation for adjust_gamma as 255 * gain * (img/255) ** gamma in the docs, where in reality it's 255.999 * gain..... I want to be accurate but also don't want to be unnecessarily confusing, and the doc does say "based on."
nairbv
force-pushed
the
adjust_gamma
branch
2 times, most recently
from
cc11df7 to
5f702bf
Compare
There was a problem hiding this comment.
The PR looks great, thanks a lot Brian!
ONNX failures are unrelated.
I just wanted to get @pmeier thoughts on the change regarding convert_image_dtype before merging this, so that we converge on the desired behavior of the function.
| @@ -161,8 +161,8 @@ def convert_image_dtype(image: torch.Tensor, dtype: torch.dtype = torch.float) - | |||
| msg = f"The cast from {image.dtype} to {dtype} cannot be performed safely." | |||
| raise RuntimeError(msg) | |||
|
|
|||
| eps = 1e-3 | |||
| return image.mul(torch.iinfo(dtype).max + 1 - eps).to(dtype) | |||
There was a problem hiding this comment.
Hum, we had quite a lot of discussion about this behavior in #2078 (comment). I believe if we make the multiplication go to dtype.max, we will end up with a non-uniform distribution over the last values.
@pmeier thoughts?
| if torch.is_floating_point(img): | ||
| return gain * result ** gamma | ||
|
|
||
| result = 255.0 * gain * (result / 255.0) ** gamma |
There was a problem hiding this comment.
For the future: we should support other integer types as well. This could probably be implemented via
convert_image_dtype(..., float)
result = torch.clamp(gain * result ** gama, 0, 1)
convert_image_dtype(..., dtype)Required by mypy, even thought technically incorrect due to possible Image parameter. torchscript doesn't support a union based type hint. Co-authored-by: vfdev <vfdev.5@gmail.com>
Add scriptable transform for adjust_gamma. Related to: #1375 #2292
BC-breaking:
off-by-one in PIL-based version, due to change in translation between float and uint8 image representations.