From ee0c5853a00dfc04a5599e44f98c06d629ac5efb Mon Sep 17 00:00:00 2001 From: Vasilis Vryniotis Date: Fri, 4 Feb 2022 12:47:28 +0000 Subject: [PATCH 1/8] Implementation commnets --- .../prototype/transforms/functional/_geometry.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/torchvision/prototype/transforms/functional/_geometry.py b/torchvision/prototype/transforms/functional/_geometry.py index c8142742fa8..a1e5b41eb4c 100644 --- a/torchvision/prototype/transforms/functional/_geometry.py +++ b/torchvision/prototype/transforms/functional/_geometry.py @@ -3,7 +3,7 @@ import torch from torchvision.prototype.features import BoundingBoxFormat from torchvision.transforms import ( # noqa: F401 - functional as _F, + functional as _F, # Shouldn't we importing `functional_tensor`? InterpolationMode, ) @@ -11,7 +11,7 @@ def horizontal_flip_image(image: torch.Tensor) -> torch.Tensor: - return image.flip((-1,)) + return image.flip((-1,)) # Why not use the _F.hflip()? def horizontal_flip_bounding_box(bounding_box: torch.Tensor, *, image_size: Tuple[int, int]) -> torch.Tensor: @@ -20,7 +20,7 @@ def horizontal_flip_bounding_box(bounding_box: torch.Tensor, *, image_size: Tupl old_format=BoundingBoxFormat.XYXY, new_format=BoundingBoxFormat.XYWH, ).unbind(-1) - x = image_size[1] - (x + w) + x = image_size[1] - (x + w) # Why not avoid formatting and do straight `boxes[:, [0, 2]] = width - boxes[:, [2, 0]]`? return convert_bounding_box_format( torch.stack((x, y, w, h), dim=-1), old_format=BoundingBoxFormat.XYWH, @@ -28,7 +28,7 @@ def horizontal_flip_bounding_box(bounding_box: torch.Tensor, *, image_size: Tupl ) -_resize_image = _F.resize +_resize_image = _F.resize # How about videos? The low level transfroms supports it def resize_image( @@ -40,7 +40,7 @@ def resize_image( ) -> torch.Tensor: new_height, new_width = size num_channels, old_height, old_width = image.shape[-3:] - batch_shape = image.shape[:-3] + batch_shape = image.shape[:-3] # In some places you use `image` to denote batches and in others `image_batch`. Should we align the names? return _resize_image( image.reshape((-1, num_channels, old_height, old_width)), size=size, From 8e8395513a76144c6ed0d53eda95aee23ec628e5 Mon Sep 17 00:00:00 2001 From: Vasilis Vryniotis Date: Fri, 4 Feb 2022 12:53:12 +0000 Subject: [PATCH 2/8] Update _augment.py --- torchvision/prototype/transforms/functional/_augment.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/torchvision/prototype/transforms/functional/_augment.py b/torchvision/prototype/transforms/functional/_augment.py index 814c34e5b00..7f322c10a75 100644 --- a/torchvision/prototype/transforms/functional/_augment.py +++ b/torchvision/prototype/transforms/functional/_augment.py @@ -15,7 +15,7 @@ def _mixup(input: torch.Tensor, batch_dim: int, lam: float, inplace: bool) -> to return input.mul_(lam).add_(input_rolled.mul_(1 - lam)) -def mixup_image(image_batch: torch.Tensor, *, lam: float, inplace: bool = False) -> torch.Tensor: +def image_batch(image_batch: torch.Tensor, *, lam: float, inplace: bool = False) -> torch.Tensor: return _mixup(image_batch, -4, lam, inplace) @@ -24,6 +24,7 @@ def mixup_one_hot_label(one_hot_label_batch: torch.Tensor, *, lam: float, inplac def cutmix_image(image: torch.Tensor, *, box: Tuple[int, int, int, int], inplace: bool = False) -> torch.Tensor: + # Shouldn't be we using `image_batch` similar to `image_batch`, to indicate that the input must be a batch? if not inplace: image = image.clone() From 5759e5069ab5d4032255ffef3dbd3cffcb310765 Mon Sep 17 00:00:00 2001 From: Vasilis Vryniotis Date: Fri, 4 Feb 2022 12:55:31 +0000 Subject: [PATCH 3/8] Update _geometry.py --- torchvision/prototype/transforms/functional/_geometry.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/torchvision/prototype/transforms/functional/_geometry.py b/torchvision/prototype/transforms/functional/_geometry.py index a1e5b41eb4c..733da291aa0 100644 --- a/torchvision/prototype/transforms/functional/_geometry.py +++ b/torchvision/prototype/transforms/functional/_geometry.py @@ -71,7 +71,7 @@ def resize_bounding_box( ) -> torch.Tensor: old_height, old_width = old_image_size new_height, new_width = new_image_size - return ( + return ( # Shouldn't we be using a low-level kernel instead, similar to above? bounding_box.view(-1, 2, 2) .mul(torch.tensor([new_width / old_width, new_height / old_height])) .view(bounding_box.shape) From 41365f5ccd22a98d070e3e0302df8ccf3c1ab337 Mon Sep 17 00:00:00 2001 From: Vasilis Vryniotis Date: Fri, 4 Feb 2022 12:56:48 +0000 Subject: [PATCH 4/8] Update _meta_conversion.py --- torchvision/prototype/transforms/functional/_meta_conversion.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/torchvision/prototype/transforms/functional/_meta_conversion.py b/torchvision/prototype/transforms/functional/_meta_conversion.py index 484066a39ee..9df5899a38a 100644 --- a/torchvision/prototype/transforms/functional/_meta_conversion.py +++ b/torchvision/prototype/transforms/functional/_meta_conversion.py @@ -4,7 +4,7 @@ def _xywh_to_xyxy(xywh: torch.Tensor) -> torch.Tensor: - xyxy = xywh.clone() + xyxy = xywh.clone() # Nice. Do we have tests to ensure that no transform does an in-place modification of input? xyxy[..., 2:] += xyxy[..., :2] return xyxy From c5b25c8a305f889ef5a327d448cfbdebe2f829fb Mon Sep 17 00:00:00 2001 From: Vasilis Vryniotis Date: Fri, 4 Feb 2022 13:00:12 +0000 Subject: [PATCH 5/8] Update _geometry.py --- torchvision/prototype/transforms/_geometry.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/torchvision/prototype/transforms/_geometry.py b/torchvision/prototype/transforms/_geometry.py index f34e5daa063..b8f5c208ea7 100644 --- a/torchvision/prototype/transforms/_geometry.py +++ b/torchvision/prototype/transforms/_geometry.py @@ -12,12 +12,12 @@ class HorizontalFlip(Transform): @staticmethod def image(input: Image) -> Image: - return Image(input.flip((-1,)), like=input) + return Image(input.flip((-1,)), like=input) # Use low-level kernel instead of directly calling flip? @staticmethod def bounding_box(input: BoundingBox) -> BoundingBox: x, y, w, h = input.convert("xywh").to_parts() - x = input.image_size[1] - (x + w) + x = input.image_size[1] - (x + w) # Use low-level kernel? return BoundingBox.from_parts(x, y, w, h, like=input, format="xywh").convert(input.format) @@ -39,7 +39,7 @@ def get_params(self, sample: Any) -> Dict[str, Any]: @staticmethod def image(input: Image, *, size: Tuple[int, int], interpolation_mode: str = "nearest") -> Image: - return Image(interpolate(input.unsqueeze(0), size, mode=interpolation_mode).squeeze(0), like=input) + return Image(interpolate(input.unsqueeze(0), size, mode=interpolation_mode).squeeze(0), like=input) # This is possible old code prior to low-level kernels? If that's the case I recommend deleting all classes that are no longer valid because this creates noise on the reviews. @staticmethod def bounding_box(input: BoundingBox, *, size: Tuple[int, int], **_: Any) -> BoundingBox: From 277d2216a66678bf38624196355cf879a64a0ef8 Mon Sep 17 00:00:00 2001 From: Vasilis Vryniotis Date: Fri, 4 Feb 2022 13:15:59 +0000 Subject: [PATCH 6/8] Update _augment.py --- torchvision/prototype/transforms/functional/_augment.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/torchvision/prototype/transforms/functional/_augment.py b/torchvision/prototype/transforms/functional/_augment.py index 7f322c10a75..3b30a22885a 100644 --- a/torchvision/prototype/transforms/functional/_augment.py +++ b/torchvision/prototype/transforms/functional/_augment.py @@ -15,7 +15,7 @@ def _mixup(input: torch.Tensor, batch_dim: int, lam: float, inplace: bool) -> to return input.mul_(lam).add_(input_rolled.mul_(1 - lam)) -def image_batch(image_batch: torch.Tensor, *, lam: float, inplace: bool = False) -> torch.Tensor: +def mixup_image(image_batch: torch.Tensor, *, lam: float, inplace: bool = False) -> torch.Tensor: return _mixup(image_batch, -4, lam, inplace) From 3594f9be53beb033080d7262f68b972bdf6a0145 Mon Sep 17 00:00:00 2001 From: Vasilis Vryniotis Date: Fri, 4 Feb 2022 13:27:19 +0000 Subject: [PATCH 7/8] Update _geometry.py --- torchvision/prototype/transforms/_geometry.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/torchvision/prototype/transforms/_geometry.py b/torchvision/prototype/transforms/_geometry.py index b8f5c208ea7..4f655c6a95c 100644 --- a/torchvision/prototype/transforms/_geometry.py +++ b/torchvision/prototype/transforms/_geometry.py @@ -7,17 +7,21 @@ from torchvision.prototype.transforms import Transform +# I recommend deleting all classes from this branch that are old to assist code reviews. +# I know that you are wokring on this already #5323 but I would try to keep your feature branch as clean as possible. +# The issue here is that these implementations are incompatible with the functional approach on the same branch which can lead to confusion. + class HorizontalFlip(Transform): NO_OP_FEATURE_TYPES = {Label} @staticmethod def image(input: Image) -> Image: - return Image(input.flip((-1,)), like=input) # Use low-level kernel instead of directly calling flip? + return Image(input.flip((-1,)), like=input) @staticmethod def bounding_box(input: BoundingBox) -> BoundingBox: x, y, w, h = input.convert("xywh").to_parts() - x = input.image_size[1] - (x + w) # Use low-level kernel? + x = input.image_size[1] - (x + w) return BoundingBox.from_parts(x, y, w, h, like=input, format="xywh").convert(input.format) @@ -39,7 +43,7 @@ def get_params(self, sample: Any) -> Dict[str, Any]: @staticmethod def image(input: Image, *, size: Tuple[int, int], interpolation_mode: str = "nearest") -> Image: - return Image(interpolate(input.unsqueeze(0), size, mode=interpolation_mode).squeeze(0), like=input) # This is possible old code prior to low-level kernels? If that's the case I recommend deleting all classes that are no longer valid because this creates noise on the reviews. + return Image(interpolate(input.unsqueeze(0), size, mode=interpolation_mode).squeeze(0), like=input) @staticmethod def bounding_box(input: BoundingBox, *, size: Tuple[int, int], **_: Any) -> BoundingBox: From dc7d0d241393a19e89e5189d28ae76e99dc0c4b0 Mon Sep 17 00:00:00 2001 From: Vasilis Vryniotis Date: Fri, 4 Feb 2022 13:46:58 +0000 Subject: [PATCH 8/8] Update _augment.py --- torchvision/prototype/transforms/functional/_augment.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/torchvision/prototype/transforms/functional/_augment.py b/torchvision/prototype/transforms/functional/_augment.py index 3b30a22885a..a0b975b998b 100644 --- a/torchvision/prototype/transforms/functional/_augment.py +++ b/torchvision/prototype/transforms/functional/_augment.py @@ -24,7 +24,7 @@ def mixup_one_hot_label(one_hot_label_batch: torch.Tensor, *, lam: float, inplac def cutmix_image(image: torch.Tensor, *, box: Tuple[int, int, int, int], inplace: bool = False) -> torch.Tensor: - # Shouldn't be we using `image_batch` similar to `image_batch`, to indicate that the input must be a batch? + # Shouldn't be we using `image_batch` similar to `mixup_image()`, to indicate that the input must be a batch? if not inplace: image = image.clone()