Allow F.normalize function to use float and list of float as mean and… #5569

YosuaMichael · 2022-03-09T05:37:26Z

Resolve issue #5046

… std

facebook-github-bot · 2022-03-09T05:37:31Z

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2022-03-09T05:37:34Z

💊 CI failures summary and remediations

As of commit 602d4a6 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

NicolasHug

Thanks for the PR @YosuaMichael ! Following up on your questions on workchat, you'll need to sign the CLA as individual. I made a few comments below, LMK what you think

NicolasHug · 2022-03-09T08:35:26Z

torchvision/transforms/functional.py



-def normalize(tensor: Tensor, mean: List[float], std: List[float], inplace: bool = False) -> Tensor:
+def normalize(tensor: Tensor, mean: Union(float, List[float]), std: Union(float, List[float]), inplace: bool = False) -> Tensor:


If you look at one of the CI jobs below

you will see that they're failing with the following error:

torchvision/transforms/transforms.py:17: in <module> from . import functional as F torchvision/transforms/functional.py:337: in <module> def normalize(tensor: Tensor, mean: Union(float, List[float]), std: Union(float, List[float]), inplace: bool = False) -> Tensor: E NameError: name 'Union' is not defined

So it looks like this is missing an import

NicolasHug · 2022-03-09T08:38:31Z

test/test_transforms.py

+    torch.manual_seed(42)
+    n_channels = 3
+    img_size = 10
+    mean = 0.5
+    std = 0.5
+    img = torch.rand(n_channels, img_size, img_size)
+    # Check that this work
+    target = F.normalize(img, mean, std)


Nit: Since we don't re-use these variable names, we don't really need to declare them. We can just shorten it to

img = torch.rand(3, 10, 10) target = F.normalize(img, mean=0.5, std=0.5)

But in reality, we don't actually need this test at all, as it wouldn't give us any error anyway, because the tests aren't checked by mypy. What we should try to do is to use mean=some_float or mean=std somewhere in our code instead. If there are no occurrence of such a thing, maybe we can just verify manually.

Note that we have a CI job for type annotations as well, called type_check_python https://app.circleci.com/pipelines/github/pytorch/vision/15467/workflows/20203b6e-c9ca-47a3-9d77-22148ff68086/jobs/1249720

datumbox · 2022-03-09T10:00:28Z

@YosuaMichael Thanks for the PR!

First of all the tests are failing because you didn't include Union in the imports. The change overall looks good but we should be very careful about merging it. I'm working on something similar on a different PR (see #5568) and JIT is giving me major headaches. My current understanding is that Union doesn't always work the way we expect. Perhaps having Union[float, List[float]] is fine but the challenge is how it handles more complex unions like Union[List[int], List[float]] (see pytorch/pytorch#69434).

facebook-github-bot · 2022-03-09T12:30:54Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

YosuaMichael · 2022-03-09T12:33:29Z

Hi @datumbox @NicolasHug thanks for the comment! Yeah, I have just read the CI error and indeed I forgot to import Union...

@NicolasHug you are right about the test! I think we dont need the unit test.
@datumbox will check on pytorch/pytorch#69434 and see if it can be somehow resolved, thanks!

Usually what is the best way to detect this import issue offline?
I have tried doing pytest test -vvv -k test_trasforms.py and flake8 torchvision/transforms/functional.py however both did not show the Union import related error.
After knowing this error, I tried to use pylint and it can detect this error, however it is too noisy as it shows quite a lot of other warnings and errors (is there any lint config used as standard?)

NicolasHug · 2022-03-09T12:40:54Z

@datumbox @YosuaMichael I'm not sure we need to worry about pytorch/pytorch#69434, at least for now. If mypy and the rest of our tests are happy with Union[float, List[float]], then I believe we're good. If mypy is not happy, then it's fine to close the issue as a no-fix.

pytest test -vvv -k test_trasforms.py

There's a typo, it should be test_transforms.py. If you ran exactly pytest test -vvv -k test_trasforms.py, then no tests actually got run.

Also, flake8 won't check types, only mypy does. So to make sure that mypy is happy with the changes, the best way is to locally write a basic file that calls F.normalize(img1, mean=0.5, std=0.5), and to check that mypy is happy with it

datumbox · 2022-03-09T12:41:11Z

After knowing this error, I tried to use pylint and it can detect this error, however it is too noisy as it shows quite a lot of other warnings and errors (is there any lint config used as standard?)

Hmmmm. Maybe you got old dependencies or something? Numpy has previously throw a bunch of warnings for me and it was because I had a very old version.

Mypy should also catch this problem. You could run it from project root with mypy --config-file mypy.ini (see guide).

datumbox

You also got a typo on the annotation (brackets vs parenthesis).

datumbox · 2022-03-09T12:42:23Z

torchvision/transforms/functional.py


-def normalize(tensor: Tensor, mean: List[float], std: List[float], inplace: bool = False) -> Tensor:
+def normalize(
+    tensor: Tensor, mean: Union(float, List[float]), std: Union(float, List[float]), inplace: bool = False


Suggested change

tensor: Tensor, mean: Union(float, List[float]), std: Union(float, List[float]), inplace: bool = False

tensor: Tensor, mean: Union[float, List[float]], std: Union[float, List[float]], inplace: bool = False

datumbox · 2022-03-09T14:25:23Z

@YosuaMichael You need to update the method on functional_tensor.py as well. This is why the test is failing.

…efore calling torch.as_tensor

YosuaMichael · 2022-03-09T15:10:58Z

Hi @datumbox , thanks for your quick response!
Previously I am not sure how to debug on my local laptop and now thanks to @NicolasHug I am able to debug it locally.

After changing functional_tensor, it seems to still give error when it call torch.as_tensor.
As of now my fix is to use isinstance and make sure the mean and std is typed as List[float] before calling torch.as_tensor. It is working in my laptop, but I will wait the CI check first if there is anything that failed.

YosuaMichael · 2022-03-09T15:48:19Z

Noted the failed test on Tuple type, and will work to fix it

…be accepted by jit

YosuaMichael · 2022-03-10T00:10:36Z

Take note that using Union[float, List[float]] as this PR suggest may have unexpected behaviour when called on torch.jit.script, for instance it will not accept tuple type as input. Here are example script to illustrate this:

import torch
import torchvision.transforms.functional as F

class TestClass_List(torch.nn.Module):
    def forward(self, img):
        return F.normalize(img, [0.5,0.5,0.5], [0.5,0.5,0.5])

class TestClass_Float(torch.nn.Module):
    def forward(self, img):
        return F.normalize(img, 0.5, 0.5)

class TestClass_Tuple(torch.nn.Module):
    def forward(self, img):
        return F.normalize(img, (0.5,0.5,0.5), (0.5,0.5,0.5))

class TestClass_Int(torch.nn.Module):
    def forward(self, img):
        return F.normalize(img, 1, 1)
  
img = torch.rand(3, 2, 2)
tc_list = TestClass_List()
tc_float = TestClass_Float()
tc_tuple = TestClass_Tuple()
tc_int = TestClass_Int()

# NOTE: Call to F.normalize without jit will work on float, int, list, and tuple
F.normalize(img, 0.5, 0.5)
F.normalize(img, 1, 1)
F.normalize(img, [0.5, 0.5, 0.5], [0.5, 0.5, 0.5])
F.normalize(img, (0.5, 0.5, 0.5), (0.5, 0.5, 0.5))

# Call with jit will work on both list and float
torch.jit.script(tc_list)
torch.jit.script(tc_float)

# Call with jit trigger error for tuple and float
try:
    torch.jit.script(tc_tuple)
except Exception as e:
    print("Tuple ERROR: {}".format(e))
try:
    torch.jit.script(tc_int)
except Exception as e:
    print("Int ERROR: {}".format(e))

As for the tuple, I think it should be resolved by using typing.Sequence instead of typing.List. However unfortunately, it is not supported as of now: https://pytorch.org/docs/stable/jit_language_reference.html#unsupported-typing-constructs.

**Note: if we use List[float] instead of Union[float, List[float]] as the type of mean in F.normalize, then it will accept tuple even when called with torch.jit.script. So I think the Union somehow remove the autocast from tuple to list in torch.jit.script.

For the int, I think it is reasonable enough not to support this because usually the value of mean and std should be floating point rather than int.

datumbox · 2022-03-10T08:27:01Z

@YosuaMichael Thanks for the detailed analysis. The reasons you mentioned above are why I'm skeptical merging this. I feel the same for my own PR at #5568. It feels like we fix something but break something else. What are your thoughts on this?

BTW have you raised a ticket on PyTorch for the specific problem? It might be worth raising it to get it fixed as this way it will make merging this PR a no-brainer.

YosuaMichael · 2022-03-10T09:49:13Z

@YosuaMichael Thanks for the detailed analysis. The reasons you mentioned above are why I'm skeptical merging this. I feel the same for my own PR at #5568. It feels like we fix something but break something else. What are your thoughts on this?

BTW have you raised a ticket on PyTorch for the specific problem? It might be worth raising it to get it fixed as this way it will make merging this PR a no-brainer.

@datumbox ,I also feel like it should not be merged because of this since it is very likely people use Tuple on mean / std.

I will raise an issue on the pytorch github soon, will update here again.

datumbox · 2022-03-10T12:22:26Z

@YosuaMichael Thanks for being an awesome contributor. Please stick around and send more PRs. :)

For exactly the same reasons you mention, I decided to also close the other PR. See #5568 (comment)

Let's close this PR, raise the issue to PyTorch core and reassess on the near future once the issue is patched.

YosuaMichael · 2022-03-10T12:45:06Z

@datumbox Raised this issue on Pytorch: pytorch/pytorch#74030

Yeah, will close this PR for now.

Allow F.normalize function to use float and list of float as mean and…

7bc9966

… std

pytorch-bot bot added the ciflow/default label Mar 9, 2022

NicolasHug reviewed Mar 9, 2022

View reviewed changes

facebook-github-bot added the cla signed label Mar 9, 2022

Fix union import and remove the unnecessary test

1b52f1b

datumbox reviewed Mar 9, 2022

View reviewed changes

Fix bracket error and format with ufmt

5d361b6

Fix the jit issue for the type by explicitly convert to List[float] b…

6eb5a62

…efore calling torch.as_tensor

YosuaMichael and others added 3 commits March 9, 2022 17:18

Transform sequence especially tuple into list on Normalize so it can …

b436a76

…be accepted by jit

Merge branch 'main' into allow-float-mean-and-std-for-normalize

4d202ed

Merge branch 'main' into allow-float-mean-and-std-for-normalize

602d4a6

datumbox closed this Mar 10, 2022

YosuaMichael mentioned this pull request Mar 10, 2022

[JIT] tuple of float is accepted in List[float] but not in Union[float, List[float]] pytorch/pytorch#74030

Open



		def normalize(tensor: Tensor, mean: List[float], std: List[float], inplace: bool = False) -> Tensor:
		def normalize(tensor: Tensor, mean: Union(float, List[float]), std: Union(float, List[float]), inplace: bool = False) -> Tensor:

	tensor: Tensor, mean: Union(float, List[float]), std: Union(float, List[float]), inplace: bool = False
	tensor: Tensor, mean: Union[float, List[float]], std: Union[float, List[float]], inplace: bool = False

Allow F.normalize function to use float and list of float as mean and… #5569

Allow F.normalize function to use float and list of float as mean and… #5569

Uh oh!

Conversation

YosuaMichael commented Mar 9, 2022

Uh oh!

facebook-github-bot commented Mar 9, 2022

Action Required

Process

Uh oh!

facebook-github-bot commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

NicolasHug Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

NicolasHug Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

datumbox commented Mar 9, 2022

Uh oh!

facebook-github-bot commented Mar 9, 2022

Uh oh!

YosuaMichael commented Mar 9, 2022

Uh oh!

NicolasHug commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

datumbox commented Mar 9, 2022

Uh oh!

datumbox left a comment

Choose a reason for hiding this comment

Uh oh!

datumbox Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

datumbox commented Mar 9, 2022

Uh oh!

YosuaMichael commented Mar 9, 2022

Uh oh!

YosuaMichael commented Mar 9, 2022

Uh oh!

YosuaMichael commented Mar 10, 2022

Uh oh!

datumbox commented Mar 10, 2022

Uh oh!

YosuaMichael commented Mar 10, 2022

Uh oh!

datumbox commented Mar 10, 2022

Uh oh!

YosuaMichael commented Mar 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

facebook-github-bot commented Mar 9, 2022 •

edited

Loading

NicolasHug commented Mar 9, 2022 •

edited

Loading