Unified Tensor/PIL crop #2342

vfdev-5 · 2020-06-23T09:20:39Z

Addresses #2292

depends on Unified Pad and F.pad opertion for PIL and Tensor inputs #2345

Description:

Unified Tensor/PIL input for RandomCrop and F.crop
Unified Tensor/PIL input for CenterCrop and F.center_crop
Unified Tensor/PIL input for FiveCrop and F.five_crop
Unified Tensor/PIL input for TenCrop and F.ten_crop

Additionally, replaced py2 style by py3 style typing.

…-2292-unify-crop-op

- crop with padding - other tests using mising private functions: _is_pil_image, _get_image_size

- sorted includes in transforms.py - used py3 annotations

vfdev-5 · 2020-06-26T13:32:51Z

torchvision/transforms/functional.py

-def ten_crop(img, size, vertical_flip=False):
-    """Generate ten cropped images from the given PIL Image.
-    Crop the given PIL Image into four corners and the central crop plus the
+def ten_crop(img: Tensor, size: List[int], vertical_flip: bool = False) -> List[Tensor]:


List[Tensor] instead of Tuple[Tensor, ..., Tensor] due to torch.jit behaviour when concat two tuples.

vfdev-5 · 2020-06-26T13:38:02Z

@fmassa could you please review this PR

fmassa

Thanks a lot for the PR Victor!

I only really have two comments (about @torch.jit.export), apart from that the PR looks good to merge, thanks!

fmassa · 2020-06-29T19:04:42Z

torchvision/transforms/functional_tensor.py

@@ -3,12 +3,19 @@
 from torch.jit.annotations import List, BroadcastingList2


-def _is_tensor_a_torch_image(input):
-    return input.ndim >= 2
+@torch.jit.export


I'm not sure @torch.jit.export is needed?

Yes, you are right about that, I'll remove it

fmassa · 2020-06-29T19:06:31Z

torchvision/transforms/functional_tensor.py



-def vflip(img):
-    # type: (Tensor) -> Tensor
+@torch.jit.export


same comment here, we now have recursive compilation, so I don't think this is actually needed. Plus, I thought that torch.jit.export was needed only for decorating nn.Module methods?

same here, i'll remove it

fmassa · 2020-06-29T19:07:05Z

torchvision/transforms/functional_tensor.py

@@ -39,12 +45,11 @@ def hflip(img):
    return img.flip(-1)


-def crop(img, top, left, height, width):
-    # type: (Tensor, int, int, int, int) -> Tensor
+def crop(img: Tensor, top: int, left: int, height: int, width: int):


nit: missing the return type?

fmassa · 2020-06-29T19:11:26Z

torchvision/transforms/transforms.py

@@ -623,7 +640,7 @@ def __call__(self, img):
        Returns:
            PIL Image: Random perspectivley transformed image.
        """
-        if not F._is_pil_image(img):
+        if not F.F_pil._is_pil_image(img):


This looks weird? Although I see the point, I'm not sure I would recommend users importing F_pil from the functional.py file.

Plus, both the functional_pil and functional_tensor are meant to be just implementation details functions, and might be preferable if they didn't leak to transforms.py

I agree, looks weird. I can introduce in functional.py as shortcut

from . import functional_pil as F_pil from . import functional_tensor as F_t _is_pil_image = F_pil._is_pil_image

sounds good to me!

- removed useless torch.jit.export - added missing typing return type - fixed F.F_pil._is_pil_image -> F._is_pil_image

vfdev-5

Thanks for the review Francisco !
I'll push modification according to the review !

vfdev-5 · 2020-06-29T19:48:15Z

torchvision/transforms/functional_tensor.py

@@ -3,12 +3,19 @@
 from torch.jit.annotations import List, BroadcastingList2


-def _is_tensor_a_torch_image(input):
-    return input.ndim >= 2
+@torch.jit.export


Yes, you are right about that, I'll remove it

vfdev-5 · 2020-06-29T19:48:27Z

torchvision/transforms/functional_tensor.py



-def vflip(img):
-    # type: (Tensor) -> Tensor
+@torch.jit.export


same here, i'll remove it

vfdev-5 · 2020-06-29T19:52:09Z

torchvision/transforms/transforms.py

@@ -623,7 +640,7 @@ def __call__(self, img):
        Returns:
            PIL Image: Random perspectivley transformed image.
        """
-        if not F._is_pil_image(img):
+        if not F.F_pil._is_pil_image(img):


I agree, looks weird. I can introduce in functional.py as shortcut

from . import functional_pil as F_pil from . import functional_tensor as F_t _is_pil_image = F_pil._is_pil_image

vfdev-5 · 2020-06-29T20:00:45Z

torchvision/transforms/functional.py

+_is_pil_image = F_pil._is_pil_image
+
+
+@torch.jit.export


Maybe event this one is unnecessary

fmassa · 2020-06-30T10:41:02Z

torchvision/transforms/functional.py

-    crop_left = int(round((image_width - crop_width) / 2.))
+
+    # crop_top = int(round((image_height - crop_height) / 2.))
+    # Result can be different between python func and scripted func


Can you explain why the results can be different? If yes this is a bug that should be reported / fixed

This is, probably, a bug with torch.jit.script:

import torch def foo(x: int) -> int: return int(round(x / 2.0)) sfoo = torch.jit.script(foo) assert foo(7) == sfoo(7) # but assert foo(5) == sfoo(5) > 2 vs 3

Thanks for the snippet, this can be further simplified as follows

import torch def foo(): return round(2.5) sfoo = torch.jit.script(foo) print(foo(), sfoo()) # 2, 3.0 # different values and types

Opened an issue in PyTorch to track this pytorch/pytorch#40771

Can you add a comment to revert back to the original implementation once that issue is fixed?

Actually, I saw that you left the code commented out, so this is fine for now

fmassa

Have 3 comments left, otherwise looks good to go!

fmassa · 2020-06-30T10:43:22Z

torchvision/transforms/functional.py


-def _is_numpy(img):
+
+@torch.jit.ignore


I think torch.jit.unused is better here, otherwise those functions won't be able to be serialized / exported

fmassa · 2020-06-30T10:43:36Z

torchvision/transforms/functional.py

    return isinstance(img, np.ndarray)


-def _is_numpy_image(img):
+@torch.jit.ignore


Same here, I think torch.jit.unused is better in this case

fmassa

Thanks a lot Victor!

* [WIP] Unified Tensor/PIL crop * Fixed misplaced type annotation * Fixed tests - crop with padding - other tests using mising private functions: _is_pil_image, _get_image_size * Unified CenterCrop and F.center_crop - sorted includes in transforms.py - used py3 annotations * Unified FiveCrop and F.five_crop * Improved tests and docs * Unified TenCrop and F.ten_crop * Removed useless typing in functional_pil * Updated code according to the review - removed useless torch.jit.export - added missing typing return type - fixed F.F_pil._is_pil_image -> F._is_pil_image * Removed useless torch.jit.export * Improved code according to the review

vfdev-5 added 8 commits June 24, 2020 14:29

[WIP] Unified Tensor/PIL crop

3025a36

Fixed misplaced type annotation

2eea2cd

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

25d67bb

…-2292-unify-crop-op

Fixed tests

c32c8bf

- crop with padding - other tests using mising private functions: _is_pil_image, _get_image_size

Unified CenterCrop and F.center_crop

69cb9c5

- sorted includes in transforms.py - used py3 annotations

Unified FiveCrop and F.five_crop

01175db

Improved tests and docs

b356e8b

Unified TenCrop and F.ten_crop

94e29a4

vfdev-5 force-pushed the issue-2292-unify-crop-op branch from 0a166fd to 94e29a4 Compare June 26, 2020 12:26

vfdev-5 marked this pull request as ready for review June 26, 2020 13:26

vfdev-5 changed the title ~~[WIP] Unified Tensor/PIL crop~~ Unified Tensor/PIL crop Jun 26, 2020

Removed useless typing in functional_pil

a74bfbc

vfdev-5 commented Jun 26, 2020

View reviewed changes

fmassa reviewed Jun 29, 2020

View reviewed changes

Updated code according to the review

8835cf5

- removed useless torch.jit.export - added missing typing return type - fixed F.F_pil._is_pil_image -> F._is_pil_image

vfdev-5 commented Jun 29, 2020

View reviewed changes

Removed useless torch.jit.export

749a938

vfdev-5 commented Jun 29, 2020

View reviewed changes

fmassa reviewed Jun 30, 2020

View reviewed changes

Improved code according to the review

dbb2295

fmassa mentioned this pull request Jun 30, 2020

[jit] round not equivalent between python and torchscript pytorch/pytorch#40771

Closed

fmassa approved these changes Jun 30, 2020

View reviewed changes

fmassa merged commit a99b6bd into pytorch:master Jun 30, 2020

vfdev-5 deleted the issue-2292-unify-crop-op branch June 30, 2020 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unified Tensor/PIL crop #2342

Unified Tensor/PIL crop #2342

vfdev-5 commented Jun 23, 2020 •

edited

Loading

vfdev-5 Jun 26, 2020

vfdev-5 commented Jun 26, 2020

fmassa left a comment

fmassa Jun 29, 2020

vfdev-5 Jun 29, 2020

fmassa Jun 29, 2020

vfdev-5 Jun 29, 2020

fmassa Jun 29, 2020

fmassa Jun 29, 2020

vfdev-5 Jun 29, 2020

fmassa Jun 29, 2020 •

edited

Loading

vfdev-5 left a comment

vfdev-5 Jun 29, 2020

vfdev-5 Jun 29, 2020

vfdev-5 Jun 29, 2020

vfdev-5 Jun 29, 2020

fmassa Jun 30, 2020

vfdev-5 Jun 30, 2020

fmassa Jun 30, 2020

fmassa Jun 30, 2020

fmassa Jun 30, 2020

fmassa left a comment

fmassa Jun 30, 2020

fmassa Jun 30, 2020

fmassa left a comment

Unified Tensor/PIL crop #2342

Unified Tensor/PIL crop #2342

Conversation

vfdev-5 commented Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

vfdev-5 commented Jun 26, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmassa Jun 29, 2020 • edited Loading

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

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

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

fmassa left a comment

Choose a reason for hiding this comment

vfdev-5 commented Jun 23, 2020 •

edited

Loading

fmassa Jun 29, 2020 •

edited

Loading