RandomRotation and fill #3303

tonystratum · 2021-01-26T19:39:13Z

Originally (vision/nightly), when torchvision.transforms.RandomRotation is used with a Tensor of shape (C, H, W), without a fill argument, both in a torch.nn.Sequential():

tensor = torch.rand(3, 224, 224)

transforms = torch.nn.Sequential(
    torchvision.transforms.Resize((224,224)),
    torchvision.transforms.Normalize(mean=[0.485, 0.456, 0.406],
                                   std=[0.229, 0.224, 0.225]),
    torchvision.transforms.RandomRotation(45)
)

transformed = transforms(tensor)

we get a following error:

Error

Traceback (most recent call last):
File "repr.py", line 13, in <module>
  transformed = transforms(tensor)
File "/home/stratum/miniconda3/envs/pytorch-cpu-nightly/lib/python3.6/site-packages/torch/nn/modules/module.py", line 889, in _call_impl
  result = self.forward(*input, **kwargs)
File "/home/stratum/miniconda3/envs/pytorch-cpu-nightly/lib/python3.6/site-packages/torch/nn/modules/container.py", line 119, in forward
  input = module(input)
File "/home/stratum/miniconda3/envs/pytorch-cpu-nightly/lib/python3.6/site-packages/torch/nn/modules/module.py", line 889, in _call_impl
  result = self.forward(*input, **kwargs)
File "/home/stratum/miniconda3/envs/pytorch-cpu-nightly/lib/python3.6/site-packages/torchvision/transforms/transforms.py", line 1236, in forward
  fill = [float(f) for f in fill]
TypeError: 'NoneType' object is not iterable

and by itself:

tensor = torch.rand(3, 224, 224)

tensor = torchvision.transforms.Resize((224,224))(tensor)
tensor = torchvision.transforms.Normalize(mean=[0.485, 0.456, 0.406],
        std=[0.229, 0.224, 0.225])(tensor)
tensor = torchvision.transforms.RandomRotation(45)(tensor)

Error

Traceback (most recent call last):
File "repr1.py", line 9, in <module>
  tensor = torchvision.transforms.RandomRotation(45)(tensor)
File "/home/stratum/miniconda3/envs/pytorch-cpu-nightly/lib/python3.6/site-packages/torch/nn/modules/module.py", line 889, in _call_impl
  result = self.forward(*input, **kwargs)
File "/home/stratum/miniconda3/envs/pytorch-cpu-nightly/lib/python3.6/site-packages/torchvision/transforms/transforms.py", line 1236, in forward
  fill = [float(f) for f in fill]
TypeError: 'NoneType' object is not iterable

The fix is to check, if img is a Tensor, and if fill is None, set it to zero.

Unit tests:

pytest test/test_transforms.py -vvv -k test_rotate

=================================== test session starts ====================================
platform linux -- Python 3.8.5, pytest-6.2.2, py-1.10.0, pluggy-0.13.1 -- /home/stratum/PycharmProjects/venvs/tv-test/bin/python
cachedir: .pytest_cache
rootdir: /home/stratum/build/myvision/vision
collected 70 items / 68 deselected / 2 selected

test/test_transforms.py::Tester::test_rotate PASSED                                  [ 50%]
test/test_transforms.py::Tester::test_rotate_fill PASSED                             [100%]

===================================== warnings summary =====================================
torchvision/__init__.py:26
/home/stratum/build/myvision/vision/torchvision/__init__.py:26: UserWarning: You are importing torchvision within its own root folder (/home/stratum/build/myvision/vision). This is not expected to work and may give errors. Please exit the torchvision project source and relaunch your python interpreter.
  warnings.warn(message.format(os.getcwd()))

-- Docs: https://docs.pytest.org/en/stable/warnings.html
======================= 2 passed, 68 deselected, 1 warning in 0.41s ========================

pytest test/test_transforms_tensor.py -vvv -k test_random_rotate

=================================== test session starts ====================================
platform linux -- Python 3.8.5, pytest-6.2.2, py-1.10.0, pluggy-0.13.1 -- /home/stratum/PycharmProjects/venvs/tv-test/bin/python
cachedir: .pytest_cache
rootdir: /home/stratum/build/myvision/vision
collected 56 items / 54 deselected / 2 selected

test/test_transforms_tensor.py::Tester::test_random_rotate PASSED                    [ 50%]
test/test_transforms_tensor.py::CUDATester::test_random_rotate SKIPPED (Skip if ...) [100%]

===================================== warnings summary =====================================
torchvision/__init__.py:26
/home/stratum/build/myvision/vision/torchvision/__init__.py:26: UserWarning: You are importing torchvision within its own root folder (/home/stratum/build/myvision/vision). This is not expected to work and may give errors. Please exit the torchvision project source and relaunch your python interpreter.
  warnings.warn(message.format(os.getcwd()))

-- Docs: https://docs.pytest.org/en/stable/warnings.html
================== 1 passed, 1 skipped, 54 deselected, 1 warning in 2.92s ==================

flake8 and mypy passed.

Hope this helps!

facebook-github-bot · 2021-01-26T19:39:17Z

Hi @tonystratum!

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 [email protected]. Thanks!

voldemortX · 2021-01-27T13:34:53Z

In summary of #3309, please not convert fill, but:

set default as fill=0 for all places.
clearly state in the docstrings that fill is no longer optional, only defaulted as 0, and check it to be a Sequence or Number.
probably no need adding tests.

torchvision/transforms/transforms.py

datumbox · 2021-01-27T14:59:46Z

@tonystratum Thanks a lot for the PR! The changes seem reasonable to me but I'll let @vfdev-5 to approve because of your discussion at #3309.

@voldemortX Thanks for all your help on reviews and for providing good input.

vfdev-5

Thanks a lot @tonystratum !
I left few comments. Sorry, I think, it would be better to set fill=0 if it was passed as None...

vfdev-5 · 2021-01-27T17:05:46Z

torchvision/transforms/transforms.py

@@ -692,6 +692,10 @@ def __init__(self, distortion_scale=0.5, p=0.5, interpolation=InterpolationMode.

        self.interpolation = interpolation
        self.distortion_scale = distortion_scale
+
+        if not isinstance(fill, (Sequence, numbers.Number)):


Maybe, we can keep BC by doing:

if fill is None: fill = 0

Sorry, I'm a bit hesitating about introducing new behaviour.

Agreed. Maybe throw a warning with it or something?

In some sense it was already done by PIL in _parse_fill* method, so let's just reassign it without a warning.

torchvision/transforms/transforms.py

codecov · 2021-01-27T17:56:33Z

Codecov Report

Merging #3303 (87d5b8c) into master (1703e4c) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3303      +/-   ##
==========================================
+ Coverage   73.93%   73.97%   +0.04%     
==========================================
  Files         104      104              
  Lines        9594     9606      +12     
  Branches     1531     1537       +6     
==========================================
+ Hits         7093     7106      +13     
+ Misses       2024     2023       -1     
  Partials      477      477

Impacted Files	Coverage Δ
torchvision/transforms/transforms.py	`84.10% <100.00%> (+0.41%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1703e4c...56fec6c. Read the comment docs.

vfdev-5

@tonystratum thanks for the update ! Could you please also include a test here: https://github.com/pytorch/vision/blob/master/test/test_transforms.py#L1527

Something like initialize a transform and verfiy then that its attribute is correctly set.
Also, for affine and perspective ops. Thanks a lot!

tonystratum · 2021-01-28T10:48:35Z

unit test reports

============================================== test session starts ===============================================
platform linux -- Python 3.8.5, pytest-6.2.2, py-1.10.0, pluggy-0.13.1 -- /home/stratum/PycharmProjects/venvs/tv-test2/bin/python
cachedir: .pytest_cache
rootdir: /home/stratum/build/myvision/vision
collected 70 items / 69 deselected / 1 selected

test/test_transforms.py::Tester::test_random_rotation PASSED                                               [100%]

================================================ warnings summary ================================================
torchvision/__init__.py:26
/home/stratum/build/myvision/vision/torchvision/__init__.py:26: UserWarning: You are importing torchvision within its own root folder (/home/stratum/build/myvision/vision). This is not expected to work and may give errors. Please exit the torchvision project source and relaunch your python interpreter.
  warnings.warn(message.format(os.getcwd()))

-- Docs: https://docs.pytest.org/en/stable/warnings.html
================================== 1 passed, 69 deselected, 1 warning in 0.30s ===================================

============================================== test session starts ===============================================
platform linux -- Python 3.8.5, pytest-6.2.2, py-1.10.0, pluggy-0.13.1 -- /home/stratum/PycharmProjects/venvs/tv-test2/bin/python
cachedir: .pytest_cache
rootdir: /home/stratum/build/myvision/vision
collected 70 items / 69 deselected / 1 selected

test/test_transforms.py::Tester::test_random_affine PASSED                                                 [100%]

================================================ warnings summary ================================================
torchvision/__init__.py:26
  /home/stratum/build/myvision/vision/torchvision/__init__.py:26: UserWarning: You are importing torchvision within its own root folder (/home/stratum/build/myvision/vision). This is not expected to work and may give errors. Please exit the torchvision project source and relaunch your python interpreter.
    warnings.warn(message.format(os.getcwd()))

-- Docs: https://docs.pytest.org/en/stable/warnings.html
================================== 1 passed, 69 deselected, 1 warning in 0.31s ===================================

============================================== test session starts ===============================================
platform linux -- Python 3.8.5, pytest-6.2.2, py-1.10.0, pluggy-0.13.1 -- /home/stratum/PycharmProjects/venvs/tv-test2/bin/python
cachedir: .pytest_cache
rootdir: /home/stratum/build/myvision/vision
collected 70 items / 68 deselected / 2 selected

test/test_transforms.py::Tester::test_randomperspective PASSED                                             [ 50%]
test/test_transforms.py::Tester::test_randomperspective_fill PASSED                                        [100%]

================================================ warnings summary ================================================
torchvision/__init__.py:26
  /home/stratum/build/myvision/vision/torchvision/__init__.py:26: UserWarning: You are importing torchvision within its own root folder (/home/stratum/build/myvision/vision). This is not expected to work and may give errors. Please exit the torchvision project source and relaunch your python interpreter.
    warnings.warn(message.format(os.getcwd()))

-- Docs: https://docs.pytest.org/en/stable/warnings.html
================================== 2 passed, 68 deselected, 1 warning in 0.32s ===================================

There was a dedicated test_randomperspective_fill for some reason, so I decided to put our tests there.

cc @vfdev-5

vfdev-5

Thanks @tonystratum, looks good ! I left a comment about NotImplemented.

Could you also fix code formatting problem: https://app.circleci.com/pipelines/github/pytorch/vision/6019/workflows/90601780-4a06-4272-b03c-bdfecf5ca3c5/jobs/389040

test/test_transforms.py

tonystratum · 2021-01-28T12:41:08Z

formatting problem

Yeah, sorry for that

vfdev-5

Looks good so far, thanks @tonystratum !

I'll merge it once the CI is done.

tonystratum · 2021-01-28T16:57:12Z

@vfdev-5 Thanks for your time!)

Summary: * initial fix * fill=0 * docstrings * fill type check * fill type check * set None to zero * unit tests * set instead of NotImplemented * fix W293 Reviewed By: datumbox Differential Revision: D26226611 fbshipit-source-id: acee01300be03ad94eab3beb1c711f5e6050f632 Co-authored-by: vfdev <[email protected]> Co-authored-by: Vasilis Vryniotis <[email protected]>

tonystratum changed the title ~~initial fix~~ RandomRotation and fill Jan 26, 2021

facebook-github-bot added the cla signed label Jan 26, 2021

initial fix

b8f6edf

tonystratum mentioned this pull request Jan 27, 2021

transforms.RandomRotation with Tensors leads to an error #3309

Closed

tonystratum force-pushed the fix_rotate branch from 65d7a85 to b8f6edf Compare January 27, 2021 13:47

voldemortX reviewed Jan 27, 2021

View reviewed changes

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

tonystratum force-pushed the fix_rotate branch from ef04f79 to a05259c Compare January 27, 2021 14:48

datumbox requested a review from vfdev-5 January 27, 2021 14:56

tonystratum and others added 5 commits January 27, 2021 17:48

fill=0

ec44069

docstrings

8c3f7f5

fill type check

a05259c

fill type check

0fcb139

Merge branch 'master' into fix_rotate

810f34b

vfdev-5 reviewed Jan 27, 2021

View reviewed changes

Merge branch 'master' into fix_rotate

cdfd400

set None to zero

93da8d3

tonystratum requested a review from vfdev-5 January 27, 2021 18:18

vfdev-5 reviewed Jan 27, 2021

View reviewed changes

vfdev-5 reviewed Jan 28, 2021

View reviewed changes

test/test_transforms.py Outdated Show resolved Hide resolved

tonystratum requested a review from vfdev-5 January 28, 2021 11:27

tonystratum commented Jan 28, 2021

View reviewed changes

test/test_transforms.py Show resolved Hide resolved

tonystratum added 2 commits January 28, 2021 14:43

unit tests

9557f0a

set instead of NotImplemented

7fb99dc

vfdev-5 approved these changes Jan 28, 2021

View reviewed changes

tonystratum and others added 3 commits January 28, 2021 16:40

fix W293

649fe85

Merge branch 'master' into fix_rotate

87d5b8c

Merge branch 'master' into fix_rotate

56fec6c

vfdev-5 merged commit c525770 into pytorch:master Jan 28, 2021

vfdev-5 mentioned this pull request Feb 15, 2023

make fill defaultdict an implementation detail #7258

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RandomRotation and fill #3303

RandomRotation and fill #3303

tonystratum commented Jan 26, 2021

facebook-github-bot commented Jan 26, 2021

voldemortX commented Jan 27, 2021

datumbox commented Jan 27, 2021

vfdev-5 left a comment

vfdev-5 Jan 27, 2021

tonystratum Jan 27, 2021

tonystratum Jan 27, 2021

vfdev-5 Jan 27, 2021

codecov bot commented Jan 27, 2021 •

edited

Loading

vfdev-5 left a comment

tonystratum commented Jan 28, 2021

vfdev-5 left a comment •

edited

Loading

tonystratum commented Jan 28, 2021

vfdev-5 left a comment •

edited

Loading

tonystratum commented Jan 28, 2021

RandomRotation and fill #3303

RandomRotation and fill #3303

Conversation

tonystratum commented Jan 26, 2021

facebook-github-bot commented Jan 26, 2021

Action Required

Process

voldemortX commented Jan 27, 2021

datumbox commented Jan 27, 2021

vfdev-5 left a comment

Choose a reason for hiding this comment

vfdev-5 Jan 27, 2021

Choose a reason for hiding this comment

tonystratum Jan 27, 2021

Choose a reason for hiding this comment

tonystratum Jan 27, 2021

Choose a reason for hiding this comment

vfdev-5 Jan 27, 2021

Choose a reason for hiding this comment

codecov bot commented Jan 27, 2021 • edited Loading

Codecov Report

vfdev-5 left a comment

Choose a reason for hiding this comment

tonystratum commented Jan 28, 2021

vfdev-5 left a comment • edited Loading

Choose a reason for hiding this comment

tonystratum commented Jan 28, 2021

vfdev-5 left a comment • edited Loading

Choose a reason for hiding this comment

tonystratum commented Jan 28, 2021

codecov bot commented Jan 27, 2021 •

edited

Loading

vfdev-5 left a comment •

edited

Loading

vfdev-5 left a comment •

edited

Loading