Unified inputs for `F.rotate` #2495

vfdev-5 · 2020-07-21T07:32:09Z

Related to #2292

Description:

Added code for F_t.rotate with test
updated F.affine tests

- updated F.affine tests

codecov · 2020-07-21T08:44:28Z

Codecov Report

Merging #2495 into master will increase coverage by 0.07%.
The diff coverage is 84.37%.

@@            Coverage Diff             @@
##           master    #2495      +/-   ##
==========================================
+ Coverage   70.73%   70.80%   +0.07%     
==========================================
  Files          94       94              
  Lines        8029     8077      +48     
  Branches     1275     1283       +8     
==========================================
+ Hits         5679     5719      +40     
- Misses       1946     1950       +4     
- Partials      404      408       +4

Impacted Files	Coverage Δ
torchvision/transforms/functional.py	`80.11% <71.42%> (-0.72%)`	⬇️
torchvision/transforms/functional_tensor.py	`67.39% <86.36%> (+2.48%)`	⬆️
torchvision/transforms/functional_pil.py	`65.02% <100.00%> (+1.18%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df6a796...a249f77. Read the comment docs.

vfdev-5 · 2020-07-24T11:05:57Z

torchvision/transforms/functional_tensor.py

+
+    point = torch.tensor([0.0, 0.0, 1.0])
+    for i in range(oh):
+        for j in range(ow):


Maybe, we should optimize grid computation, e.g. matrix of points by theta matrix multiplication.

Yes, we should optimize this otherwise the performance will be very bad.

Can't we do something like (untested and probably has bugs)

i = (torch.arange(oh) + d - oh * 0.5) / (0.5 * h) j = (torch.arange(ow) + d - ow * 0.5) / (0.5 * w) ii, jj = torch.meshgrid(i, j) coords = torch.stack([j, i], dim=-1) output_grid = torch.matmul(coords, theta.T)

Note that I removed the 3rd coordinate of point, it might be that it's necessary and we should put it back but this gives the gist of it

Thanks for the code snippet. Yes, it is something like that, I was trying to implement it from my side too :)

fmassa

Thanks a lot for the PR!

I have a few questions and comments that I think would make the code a bit simpler and faster, let me know what you think.

We can do it in two ways -- we can either merge this and then have an immediate follow-up task to optimize this function, or do it here. We can do as you prefer.

fmassa · 2020-07-29T09:19:01Z

torchvision/transforms/functional_tensor.py

+
+    point = torch.tensor([0.0, 0.0, 1.0])
+    for i in range(oh):
+        for j in range(ow):


Yes, we should optimize this otherwise the performance will be very bad.

Can't we do something like (untested and probably has bugs)

i = (torch.arange(oh) + d - oh * 0.5) / (0.5 * h) j = (torch.arange(ow) + d - ow * 0.5) / (0.5 * w) ii, jj = torch.meshgrid(i, j) coords = torch.stack([j, i], dim=-1) output_grid = torch.matmul(coords, theta.T)

Note that I removed the 3rd coordinate of point, it might be that it's necessary and we should put it back but this gives the gist of it

torchvision/transforms/functional.py

fmassa · 2020-07-29T09:32:43Z

torchvision/transforms/functional_tensor.py

+    return _apply_grid_transform(img, grid, mode)
+
+
+def _compute_output_size(theta: Tensor, w: int, h: int) -> Tuple[int, int]:


Do we need this (relatively expensive) function for calculating the output size? I think we can get it in constant time.

From some quick analysis, we might be able to get the height/width of the new image via something like

h_new = h * sin(alpha) + w * cos(alpha) w_new = h * cos(alpha) + w * sin(alpha)

(but math should be verified, and angles might be off)

Analytical approach can be nice :)
Here there matrix computations for just 4 points, so it is not that expensive. Let me check if I can provide an analytical computation with the same result as PIL.

fmassa · 2020-07-29T09:37:36Z

test/test_functional_tensor.py

@@ -418,21 +418,21 @@ def test_affine(self):
        test_configs = [
            (45, [5, 6], 1.0, [0.0, 0.0]),
            (33, (5, -4), 1.0, [0.0, 0.0]),
-            (45, [5, 4], 1.2, [0.0, 0.0]),
-            (33, (4, 8), 2.0, [0.0, 0.0]),
+            (45, [-5, 4], 1.2, [0.0, 0.0]),


Is there a particular reason why we are changing those values?

Tests were missing negative transpose values, so added just them.

vfdev-5

Thanks for the comments Francisco ! I'll update the PR and ping you once it is ready

vfdev-5 · 2020-08-03T07:00:11Z

test/test_functional_tensor.py

@@ -418,21 +418,21 @@ def test_affine(self):
        test_configs = [
            (45, [5, 6], 1.0, [0.0, 0.0]),
            (33, (5, -4), 1.0, [0.0, 0.0]),
-            (45, [5, 4], 1.2, [0.0, 0.0]),
-            (33, (4, 8), 2.0, [0.0, 0.0]),
+            (45, [-5, 4], 1.2, [0.0, 0.0]),


Tests were missing negative transpose values, so added just them.

torchvision/transforms/functional.py

vfdev-5 · 2020-08-03T07:04:03Z

torchvision/transforms/functional_tensor.py

+    return _apply_grid_transform(img, grid, mode)
+
+
+def _compute_output_size(theta: Tensor, w: int, h: int) -> Tuple[int, int]:


Analytical approach can be nice :)
Here there matrix computations for just 4 points, so it is not that expensive. Let me check if I can provide an analytical computation with the same result as PIL.

vfdev-5 · 2020-08-03T07:25:19Z

torchvision/transforms/functional_tensor.py

+
+    point = torch.tensor([0.0, 0.0, 1.0])
+    for i in range(oh):
+        for j in range(ow):


Thanks for the code snippet. Yes, it is something like that, I was trying to implement it from my side too :)

…-5/issue-2292-rotate

fmassa

Looks great, thanks a lot!

fmassa · 2020-08-05T16:10:24Z

torchvision/transforms/functional_tensor.py

+    return int(size[0]), int(size[1])
+
+
+def _expanded_affine_grid(theta: Tensor, w: int, h: int, expand: bool = False) -> Tensor:


given that we will be using our own _gen_affine_grid, I think it can be part of this function maybe?

* Added code for F_t.rotate with test - updated F.affine tests * Rotate test tolerance to 2% * Fixes failing test * Optimized _expanded_affine_grid with a single matmul op * Recoded _compute_output_size

vfdev-5 added 3 commits July 21, 2020 09:29

Added code for F_t.rotate with test

36fef0d

- updated F.affine tests

Rotate test tolerance to 2%

2b98bdc

Fixes failing test

c7231bd

vfdev-5 requested a review from fmassa July 21, 2020 08:44

vfdev-5 mentioned this pull request Jul 21, 2020

Unified inputs for T.RandomRotation #2496

Merged

vfdev-5 commented Jul 24, 2020

View reviewed changes

fmassa reviewed Jul 29, 2020

View reviewed changes

vfdev-5 commented Aug 3, 2020

View reviewed changes

vfdev-5 mentioned this pull request Aug 3, 2020

Add warning to the documentation of F_pil and F_t functions #2547

Closed

vfdev-5 added 3 commits August 3, 2020 16:15

Merge branch 'master' of https://github.com/pytorch/vision into vfdev…

44da86e

…-5/issue-2292-rotate

Optimized _expanded_affine_grid with a single matmul op

d72cb3d

Recoded _compute_output_size

a249f77

vfdev-5 requested a review from fmassa August 3, 2020 16:10

fmassa approved these changes Aug 5, 2020

View reviewed changes

fmassa merged commit 7666252 into pytorch:master Aug 5, 2020

vfdev-5 mentioned this pull request Aug 7, 2020

Unify Tensor and PIL transforms #2292

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unified inputs for `F.rotate` #2495

Unified inputs for `F.rotate` #2495

vfdev-5 commented Jul 21, 2020

codecov bot commented Jul 21, 2020 •

edited

Loading

vfdev-5 Jul 24, 2020 •

edited

Loading

fmassa Jul 29, 2020

vfdev-5 Aug 3, 2020

fmassa left a comment

fmassa Jul 29, 2020

fmassa Jul 29, 2020

vfdev-5 Aug 3, 2020

fmassa Jul 29, 2020

vfdev-5 Aug 3, 2020

vfdev-5 left a comment

vfdev-5 Aug 3, 2020

vfdev-5 Aug 3, 2020

vfdev-5 Aug 3, 2020

fmassa left a comment

fmassa Aug 5, 2020

		return _apply_grid_transform(img, grid, mode)


		def _compute_output_size(theta: Tensor, w: int, h: int) -> Tuple[int, int]:

		return int(size[0]), int(size[1])


		def _expanded_affine_grid(theta: Tensor, w: int, h: int, expand: bool = False) -> Tensor:

Unified inputs for F.rotate #2495

Unified inputs for F.rotate #2495

Conversation

vfdev-5 commented Jul 21, 2020

codecov bot commented Jul 21, 2020 • edited Loading

Codecov Report

vfdev-5 Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmassa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vfdev-5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmassa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Unified inputs for `F.rotate` #2495

Unified inputs for `F.rotate` #2495

codecov bot commented Jul 21, 2020 •

edited

Loading

vfdev-5 Jul 24, 2020 •

edited

Loading