vision: Inconsistent API for tensor and PIL image for CenterCrop

Sounds good! Please, take a look at our contributing guide which may help you with the dev setup etc. A draft PR is also good such that we can iterate over the implementation if needed and do not hesitate to ask questions here for details.

No worries and thank you for pad_symmetric, it would work fine here.

Tested on PIL version ‘6.2.1’(found this is too old, so retested on newer versions 😄) and ‘7.2.0’. And yep, there was padding in both.

Regarding ETA - I will hopefully make changes in 3/4 days(its only few hour work but working first time on vision lib, so might get delayed in setup by a little).

@vfdev-5 First, I am really sorry if I am mistaken/confused and would really appreciate for your time and explaination here.

No, PIL is incorrect also as data is still 100x100. I mean it didn’t pad the image.

There is padding happening to output and not to the input to CenterCrop method, which is expected, the input should not be changed. I say should after glossing over various transform, and I didnt see any inplace ops to modify input.

The first image I1 shows a sample image from PASCAL and CenterCrop output with cropsize larger than image size. It does the padding to output.

The second image I2 shows that even when cropsize is smaller than imagesize, input image to CenterCrop method is not modified. I only ran this test to see if my assumption was correct regarding transform not changing input.

@vfdev-5 You mentioned:-

print(transforms.CenterCrop(150)(pil_x).size) # PIL image size is 50x50

Did you mean “150x150” here?

I mean, something like

# x = PIL_image or tensor
with pytest.raises(ValueError, match=r"Crop size should be smaller than input size"):
    transforms.CenterCrop(150)(x)

PIL seems to give correct output so far, so raising error will cause backward incompatibility. Will that be fine?

No, PIL is incorrect also as data is still 100x100. I mean it didn’t pad the image. IMO, there is no BC change with raising an exception for both cases…

Fun bug with PIL and Tensors. I think it would make sense to raise an error saying that we can not crop to a larger size.

import torch
import torchvision.transforms as transforms
import torchvision.transforms.functional as TF
import numpy as np


pil_x = TF.to_pil_image(127 * torch.ones(3, 100, 100))  
print(transforms.CenterCrop(150)(pil_x).size)  # PIL image size is 50x50
print(np.asarray(pil_x).shape)

tensor_x = torch.randn(3, 100, 100)
print(transforms.CenterCrop(150)(tensor_x).shape)
> (150, 150)
> (100, 100, 3)
> torch.Size([3, 24, 24])

EDIT: My bad about pillow output size as 100x100. It is 150x150 and output image is padded.

@saurabheights Thanks for reporting and for making the issue reproducible. That’s definitely a bug. We are happy to accept a PR that solves the problem.

Concerning the proposal of extending the functionality and adjusting the API, I think this should be discussed on a separate issue because we need to assess the backward compatibility.

@vfdev-5 Any thoughts on this?

One last thing: I can work on its PR. Let me know if its alright by you.

That sounds great! Let’s wait for the green light from a member.

One last thing: I can work on its PR. Let me know if its alright by you.

P.S. A lot of the functionality needed is similar to RandomCrop, which provides flexibility of padding, so would be a good idea to consider integration of those options to CenterCrop, since beside the offset computation, rest cropping part would be same.

Applying RandomCrop on both PIL image and tensor image shows same content(but with different offset).

transforms.RandomCrop(50, pad_if_needed=True)(pil_x).show()
transforms.ToPILImage()((transforms.RandomCrop(50, pad_if_needed=True)(tensor_x))).show()

Big HP Fan 😄, He-Who-Must-Not-Be-Named.

Just when I thought I missed one spot… #3295 I added this in #3224 for now, what do you think? cc @datumbox