-
Notifications
You must be signed in to change notification settings - Fork 7k
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
adjust_hue now supports inputs of type Tensor #2566
Conversation
@CristianManta thanks for the PR, the main point of unifying inputs of |
@vfdev-5 I would be happy to help on this feature to the best of my ability. I can give it a crack. |
@CristianManta sounds great ! Could you please confirm if you could get it done by this week ?
EDIT: Forgot to say that we also have to ensure that everything is |
@vfdev-5 I can spend around 2 hours per day on it (including reading about So, if I understand correctly, I have to add a few tests in I'll read about |
@CristianManta no worries, you can start it and I can finish your PR by pushing to your branch.
yes, and I know that it wont pass as I checked that locally. That's why
(see my above message).
There is nothing complicated here, mostly the task is to fixed runtime errors while scripting the function and its subfunctions as probably several variables do not have explicitly declare their types. |
….jit.script versions.
….py in hopes to make F_t.adjust_hue scriptable...but to no avail.
@vfdev-5 I added the appropriate assertions in the tests, but of course there is an error when defining import torch
@torch.jit.script
def func(x):
return x**2
def func2(z):
return 2*func(2*z)
print(func.code)
sc2 = torch.jit.script(func2)
print(sc2.code) so I'm not sure to understand why it doesn't work with |
@CristianManta sorry for delay, yes, you need to do exactly as it is done for example for - s = cr / torch.where(eqc, maxc.new_ones(()), maxc)
+ ones = torch.ones_like(maxc)
+ s = cr / torch.where(eqc, ones, maxc)
- cr_divisor = torch.where(eqc, maxc.new_ones(()), cr)
+ cr_divisor = torch.where(eqc, ones, cr) Feel free to ask if you have other questions. |
@@ -164,7 +163,7 @@ def adjust_hue(img, hue_factor): | |||
if img.dtype == torch.uint8: | |||
img = img.to(dtype=torch.float32) / 255.0 | |||
|
|||
img = _rgb2hsv(img) | |||
img: Tensor = _rgb2hsv(img) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not necessary to add : Tensor
@@ -362,7 +361,7 @@ def _rgb2hsv(img): | |||
|
|||
cr = maxc - minc | |||
# Since `eqc => cr = 0`, replacing denominator with 1 when `eqc` is fine. | |||
s = cr / torch.where(eqc, maxc.new_ones(()), maxc) | |||
s: Tensor = cr / torch.where(eqc, maxc.new_ones(()), maxc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
Ahh I see, yes I recall that the test generated an error coming from |
…on according to PR's review.
…reased the assertLess bound to make sure that no other test fails.
…al_pil.py as well.
torchvision/transforms/functional.py
Outdated
@@ -714,7 +714,7 @@ def adjust_saturation(img: Tensor, saturation_factor: float) -> Tensor: | |||
return F_t.adjust_saturation(img, saturation_factor) | |||
|
|||
|
|||
def adjust_hue(img: Tensor, hue_factor: float) -> Tensor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically speaking, the type of hue_factor
is torch.Tensor
of dtype=whatever_torch.rand()'s_output_type_is
, but I guess it would be misleading to put hue_factor: Tensor
in the header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, since we're in functional.py
, the type of img
would be something like Union[Tensor, PIL]
, but I cannot use PIL
as a type so I left img
as it is. Not sure what to do. I don't think putting Any
is correct either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the type hints are mostly for torch.jit.script
which does not accept type mixing with Union
. Let's keep annotations as it is done for other. This is correctdef adjust_hue(img: Tensor, hue_factor: float) -> Tensor:
I think I covered all the type hint problems and I adapted the test to take into account the different range of I guess the remaining part is to fix the error bound in the Now, in order to fix it, I'm wondering what approach I should follow. The If all of this is correct, then isn't changing the assertion cheating? Shouldn't we rather change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CristianManta thanks for working on the PR ! I left some comments.
test/test_functional_tensor.py
Outdated
@@ -146,25 +148,36 @@ def test_adjustments(self): | |||
img = torch.randint(0, 256, shape, dtype=torch.uint8) | |||
|
|||
factor = 3 * torch.rand(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As factor should be a float for all ops, let's just apply .item()
on it.
test/test_functional_tensor.py
Outdated
img_pil = transforms.ToPILImage()(img) | ||
f_img_pil = f(img_pil, factor) | ||
f_img = transforms.ToTensor()(f_img_pil) | ||
for i, (f, ft, sft) in enumerate(fns): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be more simple to check the type of f
like f == F.adjust_hue
and redefine factor as factor = torch.rand(1).item() - 0.5
instead of doing if-else
with almost the same code for both cases...
torchvision/transforms/functional.py
Outdated
@@ -714,7 +714,7 @@ def adjust_saturation(img: Tensor, saturation_factor: float) -> Tensor: | |||
return F_t.adjust_saturation(img, saturation_factor) | |||
|
|||
|
|||
def adjust_hue(img: Tensor, hue_factor: float) -> Tensor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the type hints are mostly for torch.jit.script
which does not accept type mixing with Union
. Let's keep annotations as it is done for other. This is correctdef adjust_hue(img: Tensor, hue_factor: float) -> Tensor:
@@ -118,7 +118,7 @@ def adjust_saturation(img, saturation_factor): | |||
|
|||
|
|||
@torch.jit.unused | |||
def adjust_hue(img, hue_factor): | |||
def adjust_hue(img, hue_factor: float): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessary. Otherwise, you can set it as def adjust_hue(img: Any, hue_factor: float):
@@ -344,7 +344,9 @@ def _blend(img1: Tensor, img2: Tensor, ratio: float) -> Tensor: | |||
return (ratio * img1 + (1 - ratio) * img2).clamp(0, bound).to(img1.dtype) | |||
|
|||
|
|||
def _rgb2hsv(img): | |||
def _rgb2hsv(img: Tensor) -> Tensor: | |||
if not isinstance(img, torch.Tensor): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_rgb2hsv
is a private method, I think we can skip type checking and raising error here.
Btw, functional_tensor.py
module is intended to be private too, user should not use those functions in the code. Warnings will be added soon according to #2547
@@ -380,7 +383,9 @@ def _rgb2hsv(img): | |||
return torch.stack((h, s, maxc)) | |||
|
|||
|
|||
def _hsv2rgb(img): | |||
def _hsv2rgb(img: Tensor) -> Tensor: | |||
if not isinstance(img, torch.Tensor): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
test/test_functional_tensor.py
Outdated
|
||
# F uses uint8 and F_t uses float, so there is a small | ||
# difference in values caused by (at most 5) truncations. | ||
max_diff = (ft_img - f_img).abs().max() | ||
max_diff_scripted = (sft_img - f_img).abs().max() | ||
self.assertLess(max_diff, 5 / 255 + 1e-5) | ||
self.assertLess(max_diff_scripted, 5 / 255 + 1e-5) | ||
self.assertLess(max_diff, 5 + 1e-5) # TODO: 5 / 255, not 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, we can compare number of different pixels like it is done here:
vision/test/test_functional_tensor.py
Line 587 in 270e02e
num_diff_pixels = (out_tensor != out_pil_tensor).sum().item() / 3.0 |
I was thinking, firstly, to see how different two outputs (PIL and Tensor) are, do we have majority of exact values and few different values or they are all different ? If you could compute Next, we can take a look at Finally, we adapt the test such that either we compute the number of different pixels and raise an error if above some threshold. Either, rework current checking of max diff and leave comment what we check. What do you think ? |
That's a good idea, I'll compute the distances between pairs of images in the 2 norms you mentioned and this should give us a clue more about what's wrong. My suspicions is that many pixels will be different, slightly exceeding the established max difference each. I'll compute |
I fixed the mistakes that you mentioned in the previous review. I also wrote a script to get some statistics about the error differences. If you want to run it, you need to place it one level above the import torch
import vision.torchvision.transforms.functional as F
import vision.torchvision.transforms.transforms as transforms
import vision.torchvision.transforms.functional_tensor as F_t
# Parameters
tol = 1e-5
k = 20
def get_number_of_diff_pixels(img1, img2):
W = img1.shape[1]
H = img1.shape[2]
counter = 0
for i in range(W):
for j in range(H):
for ch in range(3):
if abs(img1[ch][i][j].item() - img2[ch][i][j].item()) > tol:
counter += 1
print('The discrepancy was in channel: {} for pixel ({}, {})'.format(ch, i, j))
break
return counter
tensor_img = torch.arange(3 * 5 * 5).reshape(3, 5, 5).to(torch.float) / 255
pil_img = transforms.ToPILImage()(tensor_img)
# first testing if composing the ToPILImage and its inverse produces the
# original image indeed
test_inverse_pil_img = transforms.ToTensor()(pil_img)
if (test_inverse_pil_img != tensor_img).sum().item() / 3.0 == 0:
print('ToPILImage composed with its inverse doesn\'t lose accuracy.\n')
else:
print('ToPILImage composed with its inverse loses accuracy.\n')
for i in range(k + 1):
hue_factor = -0.5 + i / k
out_tensor = F.adjust_hue(tensor_img, hue_factor)
out_pil = F.adjust_hue(pil_img, hue_factor)
out_pil_to_tensor = transforms.ToTensor()(out_pil)
# num_diff_pixels = (out_pil_to_tensor != out_tensor).sum().item() / 3.0
num_diff_pixels = get_number_of_diff_pixels(out_pil_to_tensor, out_tensor)
print('hue_factor = {}: number of different pixels = {}'.format(hue_factor, num_diff_pixels))
pct = 100.0 * num_diff_pixels / (tensor_img.shape[1] * tensor_img.shape[2])
print('percentage of pixels that are different: {}%\n'.format(pct))
print('------------------------------------------------\n')
# testing if _rgb2hsv and _hsv2rgb are precise:
hsv = F_t._rgb2hsv(tensor_img)
rgb = F_t._hsv2rgb(hsv)
diff = get_number_of_diff_pixels(rgb, tensor_img)
print('diff = {}'.format(diff)) On some relatively mild tolerance I also tested to see if the 2 conversions Sorry if the output of the script isn't pretty. I didn't have time to make it prettier |
test/test_functional_tensor.py
Outdated
scripted_fn = torch.jit.script(f) | ||
scripted_fn(img) | ||
|
||
f = transforms.ColorJitter(saturation=factor.item()) | ||
f = transforms.ColorJitter(hue=abs(hue_factor)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you use abs here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the ColorJitter
class only takes hue
inputs that are non negative:
vision/torchvision/transforms/transforms.py
Line 1024 in c547e5c
if value < 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also had to create 2 new variables, bcs_factor
and hue_factor
because after the inner loop, once factor
has been overwritten, I still needed the original factor
and hue_factor
@CristianManta I was working on the test of another color transform: |
…h by simple copy/paste and added the test_adjust_hue and ColorJitter class interface test in the same style (class interface test was removed in vfdev-5's branch for some reason).
Ok, so can I also remove |
Yes, you can remove |
… ColorJitter class interface test in test_transforms_tensor.py.
@vfdev-5 There is still one more test failing, |
@CristianManta I think there might be some changes that went in this PR that are unrelated (in the tests). About the differences from |
@fmassa I realize that since this PR was opened 12 days ago, many changes were merged into master since then, so it's worth re-trying with the new changes. Also, about the unrelated changes that went into this PR, I think it's because I copied some stuff from d016cab on which @vfdev-5 was working since his changes were related somewhat. Tonight I'll save my 4 changed files elsewhere, copy the new versions of them from master into my branch, and manually re-change only the parts that are relevant to |
@fmassa I just updated the PR and solved the conflict. I kept the d016cab style for testing the adjustments, and incorporated the I saw that, in fact, only cuda test coverage has been merged in master in
|
@vfdev-5 could you help take this PR into completion? |
… adjust_hue_tensor
- adjust hue - color jitter
Travis failed: https://travis-ci.org/github/pytorch/vision/jobs/723374281
EDIT: fixed in c58d151 |
- fixes irreproducible failing test on Travis CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more comment, otherwise looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @CristianManta and @vfdev-5 !
Hi, I think the current |
* adjust_hue now supports inputs of type Tensor * Added comparison between original adjust_hue and its Tensor and torch.jit.script versions. * Added a few type checkings related to adjust_hue in functional_tensor.py in hopes to make F_t.adjust_hue scriptable...but to no avail. * Changed implementation of _rgb2hsv and removed useless type declaration according to PR's review. * Handled the range of hue_factor in the assertions and temporarily increased the assertLess bound to make sure that no other test fails. * Fixed some lint issues with CircleCI and added type hints in functional_pil.py as well. * Corrected type hint mistakes. * Followed PR review recommendations and added test for class interface with hue. * Refactored test_functional_tensor.py to match vfdev-5's d016cab branch by simple copy/paste and added the test_adjust_hue and ColorJitter class interface test in the same style (class interface test was removed in vfdev-5's branch for some reason). * Removed test_adjustments from test_transforms_tensor.py and moved the ColorJitter class interface test in test_transforms_tensor.py. * Added cuda test cases for test_adjustments and tried to fix conflict. * Updated tests - adjust hue - color jitter * Fixes incompatible devices * Increased tol for cuda tests * Fixes potential issue with inplace op - fixes irreproducible failing test on Travis CI * Reverted fmod -> % Co-authored-by: vfdev-5 <[email protected]>
Attempts to solve the bug reported in issue #2563.
Fixes #2563
Blocked by #2586