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

Unified Pad and F.pad opertion for PIL and Tensor inputs #2345

Merged
merged 6 commits into from
Jun 26, 2020

Conversation

vfdev-5
Copy link
Collaborator

@vfdev-5 vfdev-5 commented Jun 23, 2020

Addresses #2292

Description:

@vfdev-5 vfdev-5 mentioned this pull request Jun 23, 2020
1 task
@fmassa fmassa mentioned this pull request Jun 24, 2020
16 tasks
Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a lot!

I have some minor comments, could you open a new issue so that we can track supporting other types of padding_mode in the future for Tensor?

torchvision/transforms/functional_tensor.py Outdated Show resolved Hide resolved
torchvision/transforms/functional_tensor.py Outdated Show resolved Hide resolved

Args:
img (Tensor): Image to be padded.
padding (int or tuple or list): Padding on each border. If a single int is provided this
Copy link
Member

Choose a reason for hiding this comment

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

doc should be updated, as we only support List[int] in torchscript

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, we can support both: tuple and list as tuple is casted to list below (L361). Annotation is kept as List for torchscript to pass. In the tests test_pad I check both types: [4, 4] and (2, 2, 2, 2).
Do you want me add some note about that ?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I think we should be careful with how we describe our support.
If we say we support tuple but it fails in torchscript, that's a bad user experience.

I would say it might be better to say we only support List, and as a bonus we support Tuple in non-torchscript mode.

Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we need to provide correct information about what is supported and when. In case of pad in torchscript or non-torchscipt mode, both types (tuple, list) are supported :

>>> from torchvision.transforms.functional_tensor import pad
>>> import torch
>>> tensor = torch.randint(0, 255, (3, 10, 12), dtype=torch.uint8)
>>> r1 = pad(tensor, (2, 2), fill=0, padding_mode="constant")
>>> r2 = pad(tensor, [2, 2], fill=0, padding_mode="constant")
>>> r1.eq(r2).all()
tensor(True)
>>> script_pad = torch.jit.script(pad)
>>> r1 = script_pad(tensor, (2, 2), fill=0, padding_mode="constant")
>>> r2 = script_pad(tensor, [2, 2], fill=0, padding_mode="constant")
>>> r1.eq(r2).all()
tensor(True)

Anyway, I'll update the docstring to make it crystal clear :)

torchvision/transforms/functional.py Outdated Show resolved Hide resolved
@vfdev-5
Copy link
Collaborator Author

vfdev-5 commented Jun 24, 2020

@fmassa I updated 3 of 4 comments you mentioned.

Also, it would be good to add support for some of the other methods (which are already implemented in PyTorch btw). One of the reasons why I didn't add it in my initial POC PR was because they have hard-requirements on the input tensor being 4d, so we would need to either unsqueeze / squeeze for now, or extend them in PyTorch so that they support 3d inputs as well (but that should be done with care).

It is for for another PR, right ?

@fmassa
Copy link
Member

fmassa commented Jun 24, 2020

It is for for another PR, right ?

Yes, doing it in another PR is fine, but it would be good to have an issue open to track it down so that we don't forget (and if you could work on it it would be even better :-))

@vfdev-5
Copy link
Collaborator Author

vfdev-5 commented Jun 24, 2020

Yes, doing it in another PR is fine, but it would be good to have an issue open to track it down so that we don't forget (and if you could work on it it would be even better :-))

Yep, done here : #2350

- added compatibility support for padding as [value, ] for functional_pil.pad
@vfdev-5
Copy link
Collaborator Author

vfdev-5 commented Jun 25, 2020

@fmassa I updated docstring of Pad, functional.pad, functional_pil.pad and functional_tensor.pad about what is supported for padding argument trying to unify the API between all functions.
I added compatibility support for padding as [value, ] for functional_pil.pad such that user can call

F.pad(x, padding=[1, ])
F_pil.pad(x, padding=[1, ])
F_t.pad(x, padding=[1, ])
fn = torch.jit.script(F.pad)
fn(x, padding=[1, ])

without checking if x is PIL Image or Tensor.
What do you think ?

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a lot @vfdev-5 !

@fmassa fmassa merged commit 8dc14cf into pytorch:master Jun 26, 2020
@vfdev-5 vfdev-5 deleted the pad-tensor branch June 26, 2020 09:04
de-vri-es pushed a commit to fizyr-forks/torchvision that referenced this pull request Aug 4, 2020
* [WIP] Add Tensor implementation for pad

* Unified Pad and F.pad opertion for PIL and Tensor inputs

* Added another test and improved docstring

* Updates according to the review

* Cosmetics and replaced f-string by "".format

* Updated docstring
- added compatibility support for padding as [value, ] for functional_pil.pad

Co-authored-by: Francisco Massa <fvsmassa@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants