[proto] Added mid-level ops and feature-based ops #6219

vfdev-5 · 2022-06-29T16:10:31Z

Description:

Added functional mid-level ops
Added Image/BoundingBoxes/SegmentationMask ops: Image.resize etc

Source: #6205

datumbox

Thanks @vfdev-5. Looks good overall. I did a high-level review on the patterns you used. I didn't check every single invocation and method call you make on the PR, assuming you complete the pattern (aka you call the right low-level kernel for every high-level one). I've added a few comments to get your input. Let me know what you think.

datumbox · 2022-06-30T10:27:34Z

torchvision/prototype/features/_bounding_box.py

+    def erase(self, i: int, j: int, h: int, w: int, v: torch.Tensor) -> BoundingBox:
+        raise TypeError("Erase transformation does not support bounding boxes")
+
+    def mixup(self, lam: float) -> BoundingBox:
+        raise TypeError("Mixup transformation does not support bounding boxes")
+
+    def cutmix(self, box: Tuple[int, int, int, int], lam_adjusted: float) -> BoundingBox:


I don't believe these operations should be kernels. Mixup, Cutmix etc are augmentation strategies. I would even be inclined not to add erase and let people to access the functional directly. The rational is that we should keep the kernels low. As we keep adding augmentations, we do this on the Transforms side. Thoughts?

torchvision/prototype/features/_feature.py

datumbox · 2022-06-30T10:35:35Z

torchvision/prototype/features/_feature.py

+        # How dangerous to do this instead of raising an error ?
+        return self
+
+    def resize(  # type: ignore[override]


Why ignore[override] is needed here?

there is torch.Tensor built-in deprecated op: Tensor.resize : https://github.com/pytorch/pytorch/blob/e8727994eb7cdb2ab642749d6549bc497563aa06/torch/_tensor.py#L588-L593

holly molly! that's not good. Does it make sense to rename the method? I can see that Tensor.resize_ exists still on the docs but can't find the resize.

non-inplace Tensor.resize is deprecated -> no docs. IMO, we can keep both.

Add a big TODO to get feedback on this. cc @NicolasHug thoughts?

A bit late to the party: looks like the original Tensor.resize was deprecated 4+ years ago, so I would agree that it's fine to override it IMHO.

Minor note: it's not super clear what the TODO above is about, i.e. it's not obvious what needs to be done about it?

@NicolasHug I agree that the TODO note is unclear. I'll update in a follow-up PR. The point is to fix # type: ignore[override] and make mypy happy.

Will this be actually possible? As far as I understand, mypy will throw an error as long as the 2 resize() implementations have different signatures?

This is a good question. Maybe it is impossible. I was thinking about mypy overload: https://mypy.readthedocs.io/en/stable/more_types.html#function-overloading if this could help...

torchvision/prototype/transforms/_augment.py

datumbox · 2022-06-30T10:40:49Z

torchvision/prototype/transforms/_augment.py

+        elif isinstance(inpt, PIL.Image.Image):
+            # Shouldn't we implement a fallback to tensor ?
+            raise RuntimeError("Not implemented")
+        elif isinstance(inpt, torch.Tensor):


How would the idiom would look like if we had a fallback? Do you plan to do something like the following?

elif isinstance(inpt, (PIL.Image.Image, torch.Tensor)):

we can just repeat one line:

elif isinstance(inpt, PIL.Image.Image): inpt_tensor = some_pil_to_tensor_method(inpt) output_tensor = F.erase_image_tensor(inpt_tensor, **params) return some_tensor_to_pil_method(output_tensor) elif isinstance(inpt, torch.Tensor): return F.erase_image_tensor(inpt, **params)

Sure but what if you have the kernel. I'm asking in general what idiom you will favour when you call the same F.

datumbox · 2022-06-30T10:43:29Z

torchvision/prototype/transforms/_augment.py

-            return features.OneHotLabel.new_like(input, output)
+    def _transform(self, inpt: Any, params: Dict[str, Any]) -> Any:
+        if isinstance(inpt, features._Feature):
+            return inpt.mixup(**params)


Curious to see how this will be adapted if we adopt my recommendation above to lose the augmentation specific kernels.

torchvision/prototype/transforms/functional/_geometry.py

vadimkantorov · 2022-07-02T22:35:50Z

torchvision/prototype/features/_bounding_box.py

@@ -69,3 +70,142 @@ def to_format(self, format: Union[str, BoundingBoxFormat]) -> BoundingBox:
        return BoundingBox.new_like(
            self, convert_bounding_box_format(self, old_format=self.format, new_format=format), format=format
        )
+
+    def horizontal_flip(self) -> BoundingBox:
+        from torchvision.prototype.transforms import functional as _F


Btw if this happens on a hot path, this import (even if it's not the first run) may hurt perf (from my experience of a few years back)

Yeah, this is not ideal solution. Previously I tried to add the submodule as an attribute and somehow dataloader with multiple processes hangs due to that...
If you have any better ideas, please share

Yeah that's a fair point. This workaround is temporary for as long as we work on the API. We should seek a better solution prior finalising it. Potentially a refactoring and reorganization of the modules might be a solution here but that can be decided later.

Here is an alternative approach for this at #6476

datumbox

I understand that things started piling up and it will help if we do the changes incrementally. I'm approving as in principle, I believe the approach is correct. Below I have some comments and questions for your consideration.

datumbox · 2022-07-04T14:29:41Z

torchvision/prototype/features/_feature.py

-        # Just output itself
-        # How dangerous to do this instead of raising an error ?
+    def pad(
+        self, padding: List[int], fill: Union[int, float, Sequence[float]] = 0, padding_mode: str = "constant"


Why don't we support Sequence[int] as well? Did you face JIT-scriptability issues related to how it handles seq of ints and floats? If we add support, we need to put them in all places.

I think we can add Sequence[int] to the type hint (I didn't add it as it starts looking very bulky). JIT is not concerned as pad is not scriptable (in general torch script does not recognize Sequence and we should map it to List).
In following PRs, we have to make same type hint for all fill usages.

Sounds good. You wanna put a TODO on the code or you keep track of this elsewhere?

I'll be dealing with this in Transforms PR (next one, not yet sent)

torchvision/prototype/transforms/functional/_geometry.py

torchvision/prototype/features/_segmentation_mask.py

torchvision/prototype/transforms/_augment.py

datumbox · 2022-07-04T14:39:40Z

torchvision/prototype/transforms/functional/_geometry.py

@@ -40,12 +40,58 @@ def horizontal_flip_bounding_box(
    ).view(shape)


+def horizontal_flip(inpt: Any) -> Any:


Why Any and not Union[Tensor, PIL.Image]?

If we replace Any by union[tensor, pil-image] should we keepelse branch ? In this case, it could be

def horizontal_flip(inpt: Union[torch.Tensor, PIL.Image.Image]) -> Union[torch.Tensor, PIL.Image.Image]: if isinstance(inpt, features._Feature): return inpt.horizontal_flip() elif isinstance(inpt, PIL.Image.Image): return horizontal_flip_image_pil(inpt) # elif isinstance(inpt, torch.Tensor): # return horizontal_flip_image_tensor(inpt) # else: # return inpt else: return horizontal_flip_image_tensor(inpt)

Agreed. As discussed earlier, the middle-layer shouldn't return the input as-is on else. I assume this will be removed on a follow up PR? Also we might want to play around with the order of the ifs and the type info, to see if there is any way we can make the middle layer JIT-scriptable.

datumbox · 2022-07-04T14:55:08Z

torchvision/transforms/functional_pil.py

+    perspective_coeffs: List[float],
    interpolation: int = _pil_constants.BICUBIC,
-    fill: Optional[Union[float, List[float], Tuple[float, ...]]] = 0,
+    fill: Optional[Union[float, List[float], Tuple[float, ...]]] = None,


Here is my understanding for why this is OK. The change on fill doesn't have any effect on the actual operation due to the call on _parse_fill(). Concerning the type of perspective_coeffs I believe that the change actually fixes a bug because PIL breaks if you pass a number:

>>> img.transform(img.size, Image.PERSPECTIVE, 128).show() TypeError: 'int' object is not subscriptable

torchvision/prototype/transforms/functional/_geometry.py

github-actions · 2022-07-06T07:41:14Z

Hey @vfdev-5!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

Summary: * Added mid-level ops and feature-based ops * Fixing deadlock in dataloader with circular imports * Added non-scalar fill support workaround for pad * Removed comments * int/float support for fill in pad op * Updated type hints and removed bypass option from mid-level methods * Minor nit fixes Reviewed By: jdsgomes Differential Revision: D37643902 fbshipit-source-id: e62e7274b3ead0c4e68ec5cf1fc8da7f2c0b72bf

vfdev-5 requested a review from datumbox June 29, 2022 16:10

facebook-github-bot added the cla signed label Jun 29, 2022

datumbox mentioned this pull request Jun 30, 2022

Make F.rotate/F.affine accept learnable params #5110

Open

datumbox reviewed Jun 30, 2022

View reviewed changes

vfdev-5 force-pushed the proto-f-mid-level-features-ops branch from b023a27 to 97acb26 Compare June 30, 2022 15:22

vadimkantorov reviewed Jul 2, 2022

View reviewed changes

datumbox approved these changes Jul 4, 2022

View reviewed changes

vfdev-5 force-pushed the proto-f-mid-level-features-ops branch from a81e0a1 to 5bed289 Compare July 5, 2022 16:00

vfdev-5 added 6 commits July 5, 2022 16:00

Added mid-level ops and feature-based ops

7d0057e

Fixing deadlock in dataloader with circular imports

2b3e916

Added non-scalar fill support workaround for pad

d483b16

Removed comments

8ef7b3c

int/float support for fill in pad op

c68afd6

Updated type hints and removed bypass option from mid-level methods

7b8d79b

vfdev-5 force-pushed the proto-f-mid-level-features-ops branch from 5bed289 to 7b8d79b Compare July 5, 2022 16:00

datumbox reviewed Jul 5, 2022

View reviewed changes

torchvision/prototype/transforms/functional/_geometry.py Outdated Show resolved Hide resolved

Minor nit fixes

5501fd3

vfdev-5 merged commit bd19fb8 into pytorch:main Jul 6, 2022

vfdev-5 deleted the proto-f-mid-level-features-ops branch July 6, 2022 07:40

vfdev-5 added module: transforms prototype labels Jul 6, 2022

datumbox mentioned this pull request Aug 24, 2022

Eliminate runtime cyclic dependencies #6476

Merged

		@@ -40,12 +40,58 @@ def horizontal_flip_bounding_box(
		).view(shape)


		def horizontal_flip(inpt: Any) -> Any:

[proto] Added mid-level ops and feature-based ops #6219

[proto] Added mid-level ops and feature-based ops #6219

Uh oh!

Conversation

vfdev-5 commented Jun 29, 2022

Uh oh!

datumbox left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vfdev-5 Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

datumbox left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vfdev-5 Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vfdev-5 Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

datumbox Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

vfdev-5 Jun 30, 2022 •

edited

Loading

vfdev-5 Jul 5, 2022 •

edited

Loading

vfdev-5 Jul 4, 2022 •

edited

Loading

datumbox Jul 4, 2022 •

edited

Loading