[WIP] 5522 random crop port #5555

lezwon · 2022-03-07T07:46:26Z

This PR ports the RandomCrop transform to prototype.transforms.
Linked issue: #5522

facebook-github-bot · 2022-03-07T07:46:31Z

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2022-03-07T07:46:34Z

💊 CI failures summary and remediations

As of commit 09f6b04 (more details on the Dr. CI page):

1/1 failures introduced in this PR

1 failure not recognized by patterns:

Job	Step	Action
^{lint_python_and_config}	^{Lint Python code and config files}	🔁 rerun

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

facebook-github-bot · 2022-03-07T08:28:09Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

pmeier

Thanks @lezwon for this PR! I realize you marked this as [WIP], but I have one question that we should answer before you move forward.

pmeier · 2022-03-07T08:33:55Z

torchvision/prototype/transforms/functional/_geometry.py

@@ -314,3 +314,52 @@ def resized_crop_image_pil(
 ) -> PIL.Image.Image:
    img = crop_image_pil(img, top, left, height, width)
    return resize_image_pil(img, size, interpolation=interpolation)
+
+
+def random_pad_image_tensor(


Why do we need this? Shouldn't pad_image_tensor be able to handle this? In general, we don't have kernels for random functions. All randomness should be handled in the transform.

Hi @pmeier, sorry I am a bit new to this repo. The reason I created a new function is that:

random_pad_image_tensor does not have the same logic as this function. This function takes into account the output shape required and the current image shape. I wasn't sure if this logic should be in forward.

Seeing the current code, I felt that transform-related code should be in _geometry and that there should be separate functions for tensor and PIL.

_get_params in RandomCrop requires the output from this function. So adding this logic within _transform wouldn't work as the params in _transform would not be valid.

Please do provide me with any other approach you have in mind. I could incorporate those changes.

Sorry, I misjudged the situation. I was not aware that the forward actually modified the image:

vision/torchvision/transforms/transforms.py

Lines 663 to 674 in b4cb352

if self.padding is not None:

img = F.pad(img, self.padding, self.fill, self.padding_mode)

_, height, width = F.get_dimensions(img)

# pad the width if needed

if self.pad_if_needed and width < self.size[1]:

padding = [self.size[1] - width, 0]

img = F.pad(img, padding, self.fill, self.padding_mode)

# pad the height if needed

if self.pad_if_needed and height < self.size[0]:

padding = [0, self.size[0] - height]

img = F.pad(img, padding, self.fill, self.padding_mode)

This makes things more complicated. cc @datumbox for awareness.

I would move this code into _transform. Although the structure is the same for all possible types, we still need to call different pad kernels. That would be a lot easier if we had the Pad transform from #5521 first. This way we could simply substitute pad_image_*(...) with pad(...) where pad is

pad = functools.partial( lambda image, padding: Pad( padding, fill=self.fill, padding_mode=self.padding_mode, )(image) )

and not worry about the dispatch. Thoughts?

hi @pmeier , I can keep this PR on hold and work on #5521 first, if it helps.

pmeier

sorry I am a bit new to this repo

Don't be. We are super happy that you contribute and we don't expect that you do a deep dive before starting.

I elaborated on my earlier comment inline.

torchvision/prototype/transforms/_geometry.py

pmeier · 2022-03-07T10:51:12Z

torchvision/prototype/transforms/_geometry.py

+        if isinstance(sample, features.Image):
+            output = F.random_pad_image_tensor(
+                sample,
+                output_size=self.size,
+                image_size=get_image_dimensions(sample),
+                padding=self.padding,
+                pad_if_needed=self.pad_if_needed,
+                fill=self.fill,
+                padding_mode=self.padding_mode,
+            )
+            sample = features.Image.new_like(sample, output)
+        elif isinstance(sample, PIL.Image.Image):
+            sample = F.random_pad_image_pil(
+                sample,
+                output_size=self.size,
+                image_size=get_image_dimensions(sample),
+                padding=self.padding,
+                pad_if_needed=self.pad_if_needed,
+                fill=self.fill,
+                padding_mode=self.padding_mode,
+            )
+        elif is_simple_tensor(sample):
+            sample = F.random_pad_image_tensor(
+                sample,
+                output_size=self.size,
+                image_size=get_image_dimensions(sample),
+                padding=self.padding,
+                pad_if_needed=self.pad_if_needed,
+                fill=self.fill,
+                padding_mode=self.padding_mode,
+            )


This logic should go into _transform(). Calling super.forward() here will call _transform() with all the "non-container" items that were passed in. That means, you don't need to worry about transforming lists, tuple, dictionaries or the like. input will be an individual element such as a tensor or PIL image.

Noted. Will try to incorporate this.

pmeier · 2022-03-07T11:06:17Z

torchvision/prototype/transforms/functional/_geometry.py

@@ -314,3 +314,52 @@ def resized_crop_image_pil(
 ) -> PIL.Image.Image:
    img = crop_image_pil(img, top, left, height, width)
    return resize_image_pil(img, size, interpolation=interpolation)
+
+
+def random_pad_image_tensor(


Sorry, I misjudged the situation. I was not aware that the forward actually modified the image:

vision/torchvision/transforms/transforms.py

Lines 663 to 674 in b4cb352

if self.padding is not None:

img = F.pad(img, self.padding, self.fill, self.padding_mode)

_, height, width = F.get_dimensions(img)

# pad the width if needed

if self.pad_if_needed and width < self.size[1]:

padding = [self.size[1] - width, 0]

img = F.pad(img, padding, self.fill, self.padding_mode)

# pad the height if needed

if self.pad_if_needed and height < self.size[0]:

padding = [0, self.size[0] - height]

img = F.pad(img, padding, self.fill, self.padding_mode)

This makes things more complicated. cc @datumbox for awareness.

I would move this code into _transform. Although the structure is the same for all possible types, we still need to call different pad kernels. That would be a lot easier if we had the Pad transform from #5521 first. This way we could simply substitute pad_image_*(...) with pad(...) where pad is

pad = functools.partial( lambda image, padding: Pad( padding, fill=self.fill, padding_mode=self.padding_mode, )(image) )

and not worry about the dispatch. Thoughts?

pytorch-bot bot added the ciflow/default label Mar 7, 2022

pmeier linked an issue Mar 7, 2022 that may be closed by this pull request

Port transforms.RandomCrop to prototype.transforms #5522

Closed

pmeier self-requested a review March 7, 2022 08:25

facebook-github-bot added the cla signed label Mar 7, 2022

pmeier reviewed Mar 7, 2022

View reviewed changes

pmeier mentioned this pull request Mar 7, 2022

Contribute to PyTorch project pmeier/pmeier#2

Closed

lezwon added 5 commits March 8, 2022 17:08

ported random crop to prototype.transforms

e89a31b

pad in forward

55b4efa

added to test

d986b78

fix typing issues

3cc1964

refactoring

09f6b04

lezwon force-pushed the 5522_RandomCrop_port branch from c227700 to 09f6b04 Compare March 8, 2022 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] 5522 random crop port #5555

[WIP] 5522 random crop port #5555

lezwon commented Mar 7, 2022

facebook-github-bot commented Mar 7, 2022

facebook-github-bot commented Mar 7, 2022 •

edited

Loading

facebook-github-bot commented Mar 7, 2022

pmeier left a comment

pmeier Mar 7, 2022

lezwon Mar 7, 2022

pmeier Mar 7, 2022

lezwon Mar 8, 2022

pmeier left a comment

pmeier Mar 7, 2022

lezwon Mar 8, 2022

pmeier Mar 7, 2022

	if self.padding is not None:
	img = F.pad(img, self.padding, self.fill, self.padding_mode)

	_, height, width = F.get_dimensions(img)
	# pad the width if needed
	if self.pad_if_needed and width < self.size[1]:
	padding = [self.size[1] - width, 0]
	img = F.pad(img, padding, self.fill, self.padding_mode)
	# pad the height if needed
	if self.pad_if_needed and height < self.size[0]:
	padding = [0, self.size[0] - height]
	img = F.pad(img, padding, self.fill, self.padding_mode)

[WIP] 5522 random crop port #5555

Are you sure you want to change the base?

[WIP] 5522 random crop port #5555

Conversation

lezwon commented Mar 7, 2022

facebook-github-bot commented Mar 7, 2022

Action Required

Process

facebook-github-bot commented Mar 7, 2022 • edited Loading

💊 CI failures summary and remediations

1 failure not recognized by patterns:

facebook-github-bot commented Mar 7, 2022

pmeier left a comment

Choose a reason for hiding this comment

pmeier Mar 7, 2022

Choose a reason for hiding this comment

lezwon Mar 7, 2022

Choose a reason for hiding this comment

pmeier Mar 7, 2022

Choose a reason for hiding this comment

lezwon Mar 8, 2022

Choose a reason for hiding this comment

pmeier left a comment

Choose a reason for hiding this comment

pmeier Mar 7, 2022

Choose a reason for hiding this comment

lezwon Mar 8, 2022

Choose a reason for hiding this comment

pmeier Mar 7, 2022

Choose a reason for hiding this comment

facebook-github-bot commented Mar 7, 2022 •

edited

Loading