Draft: Add adjust_hue to make it accept torch.Tensor #1460
Conversation
There was a problem hiding this comment.
Thanks for opening the PR! :)
In case that you want to indicate that you would like this to be seen as work in progress, you can either mark the PR as draft or put "[WIP]" at the beginning of the title.
You also want to add a description, and link to the reference PR by mentioning it here like so: #1375.
By the way, in you notebook, you did some tests, and you can add them in pytorch/vision/test.
README.rst
Outdated
| @@ -1,5 +1,5 @@ | |||
| torchvision | |||
| ============ | |||
There was a problem hiding this comment.
nit: This was already merged. You want to rebase on the latest changes:
git remote -v # make sure upstream is pointing to the pytorch/vision
git fetch --all
git rebase -i upstream/master
fmassa
left a comment
fmassa
left a comment
There was a problem hiding this comment.
Thanks for the PR @xiuyanni !
I did a first pass, here are some initial thoughts:
- can you add your new functionality in a new file, maybe
functional_tensor.py, which the implementations there? We still want to keep backwards-compatibility with the previous implementation (based on PIL), but we also want to be able to use torchscript with those transforms, so we should be able to separate both transforms into individual functions, and I think it makes sense to have them in different files - can you add some basic tests comparing the output of the PIL-based implementation to the Tensor-based one?
|
|
||
| def _hsv2rgb(testing): | ||
| h, s, v = testing[0], testing[1], testing[2] | ||
| i = (h * 6.0).type(torch.IntTensor) |
There was a problem hiding this comment.
using .type(torch.IntTensor is a legacy API.
Can you do instead .to(dtype=torch.int32)?
| img = Image.merge('HSV', (h, s, v)).convert(input_mode) | ||
| return img | ||
| else: | ||
| assert type(img) is torch.Tensor |
There was a problem hiding this comment.
nit: can you do instead assert isinstance(img, torch.Tensor)? It is a bit more pythonic
| img = _rgb2hsv(img) | ||
| h, s, v = img[0], img[1], img[2] | ||
| new_h = h * 255 | ||
| new_h = (new_h).type(torch.IntTensor) |
There was a problem hiding this comment.
This converts the tensor to a int32 data type, while before we had a uint8 array in the numpy implementation, so the rotation across boundaries doesn't happen.
For example, before we would have that 255 + 1 == 0, but now we have 255 + 1 == 256, which is not the same behavior.
You can probably use something like like torch.mod or torch.fmod to handle this behavior
| new_h = (new_h).type(torch.IntTensor) | ||
| new_h += int(hue_factor * 255) | ||
|
|
||
| new_h = new_h.type(torch.FloatTensor) |
|
|
||
|
|
||
| def adjust_hue(img, hue_factor): | ||
| def _rgb2hsv(testing): |
There was a problem hiding this comment.
I've had a look at the implementation from Pillow in https://github.com/python-pillow/Pillow/blob/14859ce5068ecfa750097122b61823b44d618892/src/libImaging/Convert.c#L325-L363, and from a first glance the implementations are similar, but Pillow is using the images in 0-255 range, while here the tensors are in 0-1 range. There is some normalization happening in Pillow, I wonder if it ends up being invariant to the original input ranges?
| h[h != h] = 0 | ||
| return torch.stack((h, s, v)) | ||
|
|
||
| def _hsv2rgb(testing): |
There was a problem hiding this comment.
(note to self: reference implementation in https://github.com/python-pillow/Pillow/blob/14859ce5068ecfa750097122b61823b44d618892/src/libImaging/Convert.c#L378)
| f = (h * 6.0) - i | ||
| p = v * (1.0 - s) | ||
| q = v * (1.0 - f * s) | ||
| t = v * (1.0 - (1 - f) * s) |
There was a problem hiding this comment.
I think some differences might be due to the round and clip behavior in https://github.com/python-pillow/Pillow/blob/14859ce5068ecfa750097122b61823b44d618892/src/libImaging/Convert.c#L401-L406
What about performing the operations in float, but using torch.round and torch.clamp(tensor, 0, 255)?
xiuyanni
changed the title
4 participants
No description provided.