Make crop scriptable #1379

ekagra-ranjan · 2019-09-26T22:50:49Z

This PR makes crop transform scriptable as discussed in #1375.

Fix nightly builds (pytorch#1374)

codecov-io · 2019-09-26T23:16:06Z

Codecov Report

Merging #1379 into master will decrease coverage by 0.03%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #1379      +/-   ##
==========================================
- Coverage   64.46%   64.42%   -0.04%     
==========================================
  Files          83       83              
  Lines        6421     6428       +7     
  Branches      982      984       +2     
==========================================
+ Hits         4139     4141       +2     
- Misses       1992     1995       +3     
- Partials      290      292       +2

Impacted Files	Coverage Δ
torchvision/transforms/functional_tensor.py	`57.14% <50%> (-2.86%)`	⬇️
torchvision/models/detection/roi_heads.py	`55.3% <0%> (-0.47%)`	⬇️
torchvision/models/detection/transform.py	`66.92% <0%> (ø)`	⬆️

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 53b062c...4e16aec. Read the comment docs.

fmassa

Thanks a lot for the PR @ekagra-ranjan !

I think that in order to support torchscript, we will need two (three?) functions for crop, one which only supports Tensor, the current one that supports PIL (and will be marked with a @torch.jit.ignore, and a glue function which will dispatch to one or the other, depending on the type of the input.

I'm not 100% clear on the approach yet, but I think it might make sense to create a new file containing only the scriptable transforms, first, and then we discuss at later steps how to better integrate them with the current set of transforms so that everything is as seamless as possible.

Also, once you have that, can you add tests that checks that the behavior for the different crop implementations is the same? Basically comparing the existing crop with the crop you've just implemented

ekagra-ranjan · 2019-10-17T07:57:52Z

Hi @fmassa ! Made the changes you requested.

surgan12 · 2019-10-17T06:11:50Z

test/test_functional_tensor.py

+
+        max_diff = (img_cropped_GT - img_cropped).abs().max()
+
+        assert max_diff < 5e-3, "Functional crop not working"


As pointed out by @fmassa here in #1465 and also #1483 , let's change to self.assertTrue instead of normal assert.

surgan12 · 2019-10-17T06:20:27Z

torchvision/transforms/functional_tensor.py


    if not F._is_tensor_image(img_tensor):
-        raise TypeError('tensor is not a torch image.')
+        raise TypeError('Input image is not a tensor.')


This message was the same as supplied here https://github.com/pytorch/vision/blob/53b062ca58932bbf387b96f2dd3397c4495b735b/torchvision/transforms/functional.py#L209 which already works on input tensor.
The above message would be confusing for the cases in which you supply a grayscale img tensor of the form HxW to the function. In this case the image is a tensor but not an accepted torch image but the error msg seem to mislead the user.
@ekagra-ranjan thoughts ?

The link you provided isn't working but I get your concern. It's a valid point and I will revert the change.

surgan12 · 2019-10-17T20:10:06Z

@ekagra-ranjan @fmassa, since the transforms are happening on the tensors and we would ideally don't want them to be in-place by default, should we add a boolean like we have in normalize ?

If not, I think then we should add an in-place checking test to prevent any such behaviour due to some sneaky code-err.

fmassa · 2019-10-18T14:50:55Z

@surgan12 the indexing that we perform in here indeed shares memory with the original tensors, but that's the same behavior that we already have in PIL.Image.crop, so I think this is fine.

import numpy as np, PIL

a = (np.random.rand(2, 2) > 0.5).astype(np.uint8)
i = PIL.Image.fromarray(a)
j = i.crop((0, 0, 1, 1))
aa = np.asarray(j)

print(a.flags.owndata)  # True
print(aa.flags.owndata)  # False, is a view

fmassa

Looks almost good, thanks!

I have only a couple of comments, and then this is good to merge, thanks!

fmassa · 2019-10-18T14:53:17Z

test/test_functional_tensor.py

        self.assertTrue(torch.equal(img_tensor, hflipped_img_again))

+    def test_crop(self):
+        img_tensor = torch.FloatTensor(3, 16, 16).uniform_(0, 1)


Can you use torch.rand(3, 16, 16) instead? torch.FloatTensor is a legacy API and is deprecated

fmassa · 2019-10-18T14:54:30Z

test/test_functional_tensor.py

+
+        max_diff = (img_cropped_GT - img_cropped).abs().max()
+
+        self.assertLess(max_diff, 5e-3, "functional_tensor crop not working")


Can't we make the difference 0? Is this because we need to perform casting between types, which gives some accuracy differences?
In that case, can't you directly make img_tensor be a uint8 tensor with values from 0-255?

surgan12 · 2019-10-18T15:01:56Z

@fmassa, yes makes sense.

fmassa

Thanks!

ekagra-ranjan added 3 commits September 27, 2019 03:56

Merge pull request #2 from pytorch/master

2ce74ea

Fix nightly builds (pytorch#1374)

Make crop torchscriptable

1b94441

Relevant pytorch#1375

Invert x and y axis

38ef37a

fix lint

05b2787

fmassa requested changes Sep 30, 2019

View reviewed changes

ekagra-ranjan added 8 commits October 17, 2019 01:54

sync

9353085

Merge branch 'pytorch-master' into mz-script-transform

58b2240

Add crop test

6f54c3b

revert deletion of space in functional

4be6334

add import random

9634785

add dimension in doc

7c5b4a6

add import

1237fcc

fix flake8

a871402

surgan12 reviewed Oct 17, 2019

View reviewed changes

change to self.assert*

12b8e26

fmassa requested changes Oct 18, 2019

View reviewed changes

ekagra-ranjan added 3 commits October 18, 2019 21:42

convert to uint8

bf12955

assertTrue

150c96f

lint

4e16aec

fmassa approved these changes Oct 18, 2019

View reviewed changes

fmassa merged commit d194082 into pytorch:master Oct 18, 2019


		max_diff = (img_cropped_GT - img_cropped).abs().max()

		assert max_diff < 5e-3, "Functional crop not working"


		max_diff = (img_cropped_GT - img_cropped).abs().max()

		self.assertLess(max_diff, 5e-3, "functional_tensor crop not working")

Make crop scriptable #1379

Make crop scriptable #1379

Uh oh!

Conversation

ekagra-ranjan commented Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fmassa left a comment

Choose a reason for hiding this comment

Uh oh!

ekagra-ranjan commented Oct 17, 2019

Uh oh!

surgan12 Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surgan12 Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

ekagra-ranjan Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

surgan12 commented Oct 17, 2019

Uh oh!

fmassa commented Oct 18, 2019

Uh oh!

fmassa left a comment

Choose a reason for hiding this comment

Uh oh!

fmassa Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

fmassa Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

ekagra-ranjan Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

surgan12 commented Oct 18, 2019

Uh oh!

fmassa left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ekagra-ranjan commented Sep 26, 2019 •

edited

Loading

codecov-io commented Sep 26, 2019 •

edited

Loading

surgan12 Oct 17, 2019 •

edited

Loading