Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add tests for F.crop and transforms.RandomCrop #7892

Merged
merged 11 commits into from
Aug 30, 2023
Merged

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Aug 28, 2023

@pytorch-bot
Copy link

pytorch-bot bot commented Aug 28, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7892

Note: Links to docs will display an error until the docs builds have been completed.

❌ 13 New Failures, 1 Pending, 19 Unrelated Failures

As of commit 5e1510b with merge base 58f834a (image):

NEW FAILURES - The following jobs have failed:

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

test/test_transforms_v2_refactored.py Show resolved Hide resolved
# left, right, top, bottom side
dict(top=-5, left=-5, height=30, width=10),
dict(top=-5, left=5, height=30, width=10),
dict(top=-5, left=-5, height=20, width=20),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be 5 since height is 20?

I'm not sure I understand the "left, right, top, bottom side" and "left, right, top, bottom corner" notations though. Nor why we need so many configurations.

Suggested change
dict(top=-5, left=-5, height=20, width=20),
dict(top=5, left=-5, height=20, width=20),

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F.crop implicitly pads when we have a negative anchor or the crop size is larger than the input size

if left < 0 or top < 0 or right > w or bottom > h:
image = image[..., max(top, 0) : bottom, max(left, 0) : right]
torch_padding = [
max(min(right, 0) - left, 0),
max(right - max(w, left), 0),
max(min(bottom, 0) - top, 0),
max(bottom - max(h, top), 0),
]
return _pad_with_scalar_fill(image, torch_padding, fill=0, padding_mode="constant")

Although I dislike it, we need to keep BC for it.

I tried to enumerate all possible cases here:

  • valid crop, i.e. cropped region is fully on the inside
  • cropped region is strictly larger than image, i.e. we need padding on all sides
  • cropped region only contains one side of the image (4 times for each side)
  • cropped region only contains a corner of the image (4 times for each corner)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a small snippet to verify the configurations listed here are actually correct:

import torch
from torchvision.transforms.v2 import functional as F

INPUT_SIZE = (21, 11)

CORRECTNESS_CROP_KWARGS = [
    # center
    dict(top=5, left=5, height=10, width=5),
    # larger than input, i.e. pad
    dict(top=-5, left=-5, height=30, width=20),
    # sides: left, right, top, bottom
    dict(top=-5, left=-5, height=30, width=10),
    dict(top=-5, left=5, height=30, width=10),
    dict(top=-5, left=-5, height=20, width=20),
    dict(top=5, left=-5, height=20, width=20),
    # corners: top-left, top-right, bottom-left, bottom-right
    dict(top=-5, left=-5, height=20, width=10),
    dict(top=-5, left=5, height=20, width=10),
    dict(top=5, left=-5, height=20, width=10),
    dict(top=5, left=5, height=20, width=10),
]

TITLE_WITH_KWARGS = zip(
    [
        "center",
        "pad",
        *[f"{side}_side" for side in ["left", "right", "top", "bottom"]],
        *[f"{corner}_corner" for corner in ["top_left", "top_right", "bottom_left", "bottom_right"]],
    ],
    CORRECTNESS_CROP_KWARGS,
)


image = torch.full((3, *INPUT_SIZE), 255, dtype=torch.uint8)

for title, kwargs in TITLE_WITH_KWARGS:
    F.to_pil_image(F.crop_image(image, **kwargs)).save(f"{title}.png")

test/test_transforms_v2_refactored.py Outdated Show resolved Hide resolved
test/test_transforms_v2_refactored.py Outdated Show resolved Hide resolved
Comment on lines +2582 to +2583
if isinstance(input, PIL.Image.Image) and isinstance(value, (tuple, list)) and len(value) == 1:
pytest.xfail("F._pad_image_pil does not support sequences of length 1 for fill.")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably add support for this. Note that this is not a regression:

_pad_image_pil = _register_kernel_internal(pad, PIL.Image.Image)(_FP.pad)

@@ -1165,7 +1165,7 @@ def pad_image(
fill: Optional[Union[int, float, List[float]]] = None,
padding_mode: str = "constant",
) -> torch.Tensor:
# Be aware that while `padding` has order `[left, top, right, bottom]` has order, `torch_padding` uses
# Be aware that while `padding` has order `[left, top, right, bottom]`, `torch_padding` uses
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Driveby

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Philip, LGTM if green

test/test_transforms_v2_refactored.py Outdated Show resolved Hide resolved
test/test_transforms_v2_refactored.py Outdated Show resolved Hide resolved
@pmeier pmeier merged commit a06df0d into pytorch:main Aug 30, 2023
11 of 32 checks passed
@pmeier pmeier deleted the port/crop branch August 30, 2023 14:01
facebook-github-bot pushed a commit that referenced this pull request Sep 6, 2023
Summary: Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

Reviewed By: matteobettini

Differential Revision: D48900371

fbshipit-source-id: e3d5e30e4cdff66416480798360b69eb8a2655e7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants