remove unnecessary checks from pad_image_tensor #6894
Conversation
| @@ -645,7 +645,32 @@ def rotate( | |||
| return rotate_image_pil(inpt, angle, interpolation=interpolation, expand=expand, fill=fill, center=center) | |||
|
|
|||
|
|
|||
| pad_image_pil = _FP.pad | |||
| def _parse_pad_padding(padding: Union[int, List[int]]) -> List[int]: | |||
There was a problem hiding this comment.
I've merged the check for invalid types as well as wrong lengths here, since this function is also used by pad_bounding_box and that currently doesn't have these checks.
| new_height = height + top + bottom | ||
| new_width = width + left + right | ||
|
|
||
| return image.reshape(shape[:-3] + (num_channels, new_height, new_width)) | ||
|
|
||
|
|
||
| # TODO: This should be removed once pytorch pad supports non-scalar padding values | ||
| # TODO: This should be removed once torch_pad supports non-scalar padding values |
There was a problem hiding this comment.
@vfdev-5 Is there an issue for that?
| @@ -711,6 +771,9 @@ def _pad_with_vector_fill( | |||
| return output | |||
|
|
|||
|
|
|||
| pad_image_pil = _FP.pad | |||
There was a problem hiding this comment.
Minor cleanup, since we normally define the PIL kernel below the tensor one.
| elif isinstance(fill, (int, float)) or len(fill) == 1: | ||
| fill_number = fill[0] if isinstance(fill, list) else fill | ||
| return _pad_with_scalar_fill(image, padding, fill=fill_number, padding_mode=padding_mode) | ||
| return _pad_with_scalar_fill(image, torch_padding, fill=0, padding_mode=padding_mode) |
There was a problem hiding this comment.
Why fill=0 instead of fill=None as originally ? Do we still need this workaround ?
There was a problem hiding this comment.
Previously, this was handled by _FT.pad:
vision/torchvision/transforms/functional_tensor.py
Lines 386 to 387 in e64784c
Since we are no longer calling it, we need to handle it our own. For some reason, the fill overwrite above did not work for me while developing and so I went this way. Rechecking today, it seems to work and it was probably caused by something else. I've fixed this in 1622cd6.
More general though, why are we allowing None in the first place and even use it as default if we all we do with it is to map it to 0? I faintly remember there were discussions about it, but I think I was out of the loop on them. Is there a public discussion that you can point me to or did this happen offline? Intuitively, I would remove the None as valid value and just use 0 as default.
Summary: * remove unnecessary changes from pad_image_tensor * cleanup * fix fill=None workaround * address review comments * remove more xfails Reviewed By: datumbox Differential Revision: D41020544 fbshipit-source-id: d677ea0dd79f8e8055ed7c36a65a0bb980e3b578
Closes #6882. As discussed offline, since our implementation already only converts the dtype for non-constant padding
vision/torchvision/transforms/functional_tensor.py
Lines 426 to 431 in 1921613
there is not much performance to gain on our side. As explained in #6818 (comment), the two possible optimization vectors are edge cases and have to happen in PyTorch core and thus would affect v1 as well as v2.
Thus, this PR is mostly refactoring.
cc @vfdev-5 @datumbox @bjuncek