Added typing annotations to transforms/functional_pil #4234

oke-aditya · 2021-07-30T18:40:21Z

Following up on #2025, this PR adds missing typing annotations in functional_pil.py

Any feedback is welcome!

cc @frgfm @pmeier

oke-aditya

Little help would be nice! I'm not sure about few types.

Edit: CI appears green

torchvision/transforms/functional_pil.py

pmeier

Hey @oke-aditya and sorry for the delay. I have some comments inline.

torchvision/transforms/functional_pil.py

…yping4

oke-aditya

I think I have fixed the bugs.
Should we unblock this file from mypy.ini ?

Edit: Nope. I think it's not yet all green.

We have stumbled upon mypy/#3186 issue because of which mypy still complains.
It thinks int is not a number and hence we cannot assign 0 as default arg.

Edit2: A small workaround is to use

Optional[Union[int, float, List[int], List[float], Tuple[int, ...], Tuple[float, ...]]]

It sounds slightly more verbose. Probably you know a better a way 😄

oke-aditya

Just marking the lines where error occured

torchvision/transforms/functional_pil.py

oke-aditya · 2021-08-16T19:46:08Z

Hmmm tests seem to be failing on .pad() function.
Not very sure why it occured, but I think that has easy fix by passing args as padding=pad.

Also another point I noticed that.

The functional tensor has similar padding code, and I think that it accepts int as well as List and Tuples of them.
(Basically same as Union[int, List[int], Tuple[int, ...]],)

But the typing seems to be only List[int].

I think that the typing for Functional PIL and Functional Tensor should be same. Since that's how we test them as well.

pmeier

We have stumbled upon mypy/#3186 issue because of which mypy still complains.
It thinks int is not a number and hence we cannot assign 0 as default arg.

Good catch. I've added a fix inline.

Hmmm tests seem to be failing on .pad() function.
Not very sure why it occured, but I think that has easy fix by passing args as padding=pad.

Fix inline.

The functional tensor has similar padding code, and I think that it accepts int as well as List and Tuples of them.
(Basically same as Union[int, List[int], Tuple[int, ...]],)

But the typing seems to be only List[int].

I think that the typing for Functional PIL and Functional Tensor should be same. Since that's how we test them as well.

Yes the signatures and in turn the annotations should be the same. Would you send a follow-up PR after this one is merged?

torchvision/transforms/functional_pil.py

oke-aditya · 2021-08-17T14:27:24Z

@datumbox @pmeier
I think so CI is green enough to merge this. Probably all the failures are unrelated.

Hopefully, Good to go!

Edit:

Leaving leaving your comment as a new issue.

datumbox

@oke-aditya I checked this again. Left a couple of comments to get your input. It's mainly around whether we want to align the API between PIL and Tensor or if we want to keep the two separate. No strong opinions here. @pmeier Let me also know what you think please.

I know this PR turned out to be lengthy so if you had enough we can address them later. Cheers! :)

datumbox · 2021-08-17T14:38:49Z

torchvision/transforms/functional_pil.py

+def pad(
+    img: Image.Image,
+    padding: Union[int, List[int], Tuple[int, ...]],
+    fill: Optional[Union[float, List[float], Tuple[float, ...]]] = 0,


This is defined as int on the tensor equivalent method, though it supports both. Do we want them aligned?

I think we want float in tensor equivalent method to align this.

no strong opinion here. I leave it up to you.

datumbox · 2021-08-17T14:40:27Z

torchvision/transforms/functional_pil.py

-def resize(img, size, interpolation=Image.BILINEAR, max_size=None):
+def resize(
+    img: Image.Image,
+    size: Union[Sequence[int], int],


The tensor API uses List instead of Sequence. Do we want them aligned?

Yes, I think we need to have Sequence in tensor, to align. But probably JIT does not support it.

Yes, torch.jit does not support Sequence. Since we also have a Union here, we can't align them anyway. Thus, I think it is ok to go with Sequence.

torchvision/transforms/functional_pil.py

datumbox · 2021-08-17T14:42:40Z

torchvision/transforms/functional_pil.py

+    img: Image.Image,
+    matrix: List[float],
+    interpolation: int = 0,
+    fill: Optional[Union[float, List[float], Tuple[float, ...]]] = 0,


FYI strangely the tensor API defines this as float as well.

Actually the tensor API defines it to be only List[float] and not all the three, again something which isn't aligned or probably has an issue.

It's the union that is not supported. I just highlight that here the type (float vs int) is actually aligned. Which is weird cause we don't seem to keep things aligned. I probably agree that it's the tensor that needs changing though.

I just verified Sequence is not supported by JIT. And yes Union too is not supported.

https://pytorch.org/docs/stable/jit_language_reference.html#supported-type

datumbox · 2021-08-17T14:44:12Z

torchvision/transforms/functional_pil.py

+def affine(
+    img: Image.Image,
+    matrix: List[float],
+    interpolation: int = 0,


Perhaps we should use Image.NEAREST instead of the numeric here?

Hmm, something stranger is that in the tensor API we pass a string.
I'm not sure how would we align it. (again probably JIT?)

I mean something different. See the next comment (enum-style VS integer for the default value).

torchvision/transforms/functional_pil.py

oke-aditya

Hey @datumbox ! Here are my thoughts.

There are probably multiple places where we have misaligned with functional_tensor.

My idea was to follow-up this with a new PR. (#4282 ).

Let me know what you think!

oke-aditya · 2021-08-17T15:47:04Z

torchvision/transforms/functional_pil.py

+def pad(
+    img: Image.Image,
+    padding: Union[int, List[int], Tuple[int, ...]],
+    fill: Optional[Union[float, List[float], Tuple[float, ...]]] = 0,


I think we want float in tensor equivalent method to align this.

oke-aditya · 2021-08-17T15:47:07Z

torchvision/transforms/functional_pil.py

-def resize(img, size, interpolation=Image.BILINEAR, max_size=None):
+def resize(
+    img: Image.Image,
+    size: Union[Sequence[int], int],


Yes, I think we need to have Sequence in tensor, to align. But probably JIT does not support it.

oke-aditya · 2021-08-17T15:48:06Z

torchvision/transforms/functional_pil.py

+def affine(
+    img: Image.Image,
+    matrix: List[float],
+    interpolation: int = 0,


Hmm, something stranger is that in the tensor API we pass a string.
I'm not sure how would we align it. (again probably JIT?)

oke-aditya · 2021-08-17T15:49:31Z

torchvision/transforms/functional_pil.py

+    img: Image.Image,
+    matrix: List[float],
+    interpolation: int = 0,
+    fill: Optional[Union[float, List[float], Tuple[float, ...]]] = 0,


Actually the tensor API defines it to be only List[float] and not all the three, again something which isn't aligned or probably has an issue.

torchvision/transforms/functional_pil.py

…yping4

oke-aditya · 2021-08-17T17:29:16Z

I think this should be do it! @datumbox can you just cross check if I have kept the modes BILINEAR CUBIC and NEAREST correct as per old definitions?

datumbox

LGTM, thank you @oke-aditya for your contribution and for your patience. :)

I checked the modes and they look fine to me.

@pmeier Do you mind also having a look that we are good to go? Once I get your stamp we can merge.

pmeier

can you just cross check if I have kept the modes BILINEAR CUBIC and NEAREST correct as per old definitions?

Seems to be good, even replacing an old 0 with NEAREST 👍

LGTM, thanks a lot @oke-aditya!

pmeier · 2021-08-18T05:09:43Z

torchvision/transforms/functional_pil.py

-def resize(img, size, interpolation=Image.BILINEAR, max_size=None):
+def resize(
+    img: Image.Image,
+    size: Union[Sequence[int], int],


Yes, torch.jit does not support Sequence. Since we also have a Union here, we can't align them anyway. Thus, I think it is ok to go with Sequence.

github-actions · 2021-08-18T08:32:58Z

Hey @datumbox!

You merged this PR, but no labels were added.

Summary: * fix * add functional PIL typings * fix types * fix types * fix a small one * small fix * fix type * fix interpolation types Reviewed By: NicolasHug Differential Revision: D30417195 fbshipit-source-id: 5e09a14011e5cca76d87c2a3dfc2872303f40b2c Co-authored-by: Vasilis Vryniotis <[email protected]>

oke-aditya added 13 commits May 20, 2021 12:47

fix

b2f6615

Merge branch 'master' of https://github.com/pytorch/vision

4fb038d

Merge branch 'master' of https://github.com/pytorch/vision

deda5d7

Merge branch 'master' of https://github.com/pytorch/vision

5490821

Merge branch 'master' of https://github.com/pytorch/vision

4cfc220

Merge branch 'master' of https://github.com/pytorch/vision

6306746

Merge branch 'master' of https://github.com/pytorch/vision

e8c93cf

Merge branch 'master' of https://github.com/pytorch/vision

6871ccc

Merge branch 'master' of https://github.com/pytorch/vision

80060bf

Merge branch 'master' of https://github.com/pytorch/vision

3b5f0ca

Merge branch 'master' of https://github.com/pytorch/vision

0457e6d

add functional PIL typings

fa9eb08

fix types

1e6dd9e

facebook-github-bot added the cla signed label Jul 30, 2021

oke-aditya commented Jul 30, 2021

View reviewed changes

frgfm mentioned this pull request Jul 31, 2021

Type annotations #2025

Closed

pmeier reviewed Aug 16, 2021

View reviewed changes

datumbox reviewed Aug 16, 2021

View reviewed changes

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

oke-aditya added 3 commits August 17, 2021 00:19

Merge branch 'master' of https://github.com/pytorch/vision into add_t…

18f28e7

…yping4

fix types

9d294f8

fix a small one

6bfd004

oke-aditya commented Aug 16, 2021

View reviewed changes

oke-aditya requested review from datumbox and pmeier August 16, 2021 19:06

small fix

0dce853

oke-aditya commented Aug 16, 2021

View reviewed changes

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

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

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

pmeier reviewed Aug 17, 2021

View reviewed changes

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

fix type

b6e1638

oke-aditya requested a review from pmeier August 17, 2021 14:27

oke-aditya mentioned this pull request Aug 17, 2021

Check Type Annotations for transforms/functional_pil and transforms/functional_tensor #4282

Open

datumbox reviewed Aug 17, 2021

View reviewed changes

oke-aditya commented Aug 17, 2021

View reviewed changes

oke-aditya added 2 commits August 17, 2021 22:55

fix interpolation types

3de8a93

Merge branch 'master' of https://github.com/pytorch/vision into add_t…

6c94b47

…yping4

oke-aditya requested a review from datumbox August 17, 2021 17:27

datumbox approved these changes Aug 17, 2021

View reviewed changes

pmeier approved these changes Aug 18, 2021

View reviewed changes

Merge branch 'master' into add_typing4

b3cb4c4

datumbox merged commit 759c5b6 into pytorch:master Aug 18, 2021

datumbox added code quality module: transforms labels Aug 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added typing annotations to transforms/functional_pil #4234

Added typing annotations to transforms/functional_pil #4234

oke-aditya commented Jul 30, 2021 •

edited

Loading

oke-aditya left a comment •

edited

Loading

pmeier left a comment

oke-aditya left a comment •

edited

Loading

oke-aditya left a comment

oke-aditya commented Aug 16, 2021 •

edited

Loading

pmeier left a comment

oke-aditya commented Aug 17, 2021 •

edited

Loading

datumbox left a comment

datumbox Aug 17, 2021

oke-aditya Aug 17, 2021

datumbox Aug 17, 2021

datumbox Aug 17, 2021

oke-aditya Aug 17, 2021

pmeier Aug 18, 2021

datumbox Aug 17, 2021

oke-aditya Aug 17, 2021

datumbox Aug 17, 2021

oke-aditya Aug 17, 2021

datumbox Aug 17, 2021 •

edited

Loading

oke-aditya Aug 17, 2021

datumbox Aug 17, 2021 •

edited

Loading

oke-aditya left a comment

oke-aditya Aug 17, 2021

oke-aditya Aug 17, 2021

oke-aditya Aug 17, 2021

oke-aditya Aug 17, 2021

oke-aditya commented Aug 17, 2021

datumbox left a comment

pmeier left a comment

pmeier Aug 18, 2021

github-actions bot commented Aug 18, 2021

Added typing annotations to transforms/functional_pil #4234

Added typing annotations to transforms/functional_pil #4234

Conversation

oke-aditya commented Jul 30, 2021 • edited Loading

oke-aditya left a comment • edited Loading

Choose a reason for hiding this comment

pmeier left a comment

Choose a reason for hiding this comment

oke-aditya left a comment • edited Loading

Choose a reason for hiding this comment

oke-aditya left a comment

Choose a reason for hiding this comment

oke-aditya commented Aug 16, 2021 • edited Loading

pmeier left a comment

Choose a reason for hiding this comment

oke-aditya commented Aug 17, 2021 • edited Loading

datumbox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datumbox Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datumbox Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

oke-aditya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oke-aditya commented Aug 17, 2021

datumbox left a comment

Choose a reason for hiding this comment

pmeier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 18, 2021

oke-aditya commented Jul 30, 2021 •

edited

Loading

oke-aditya left a comment •

edited

Loading

oke-aditya left a comment •

edited

Loading

oke-aditya commented Aug 16, 2021 •

edited

Loading

oke-aditya commented Aug 17, 2021 •

edited

Loading

datumbox Aug 17, 2021 •

edited

Loading

datumbox Aug 17, 2021 •

edited

Loading