Making Vflip and Hflip in Tensor format #1465

surgan12 · 2019-10-15T14:09:20Z

fmassa

Thanks a lot for the PR @surgan12, it looks great !

I have a few comments, let me know what you think.

Also, I think lint will fail, can you check?

fmassa · 2019-10-15T14:12:16Z

test/test_functional_tensor.py

+		vflipped_img = F_t.vflip(img_tensor)
+		vflipped_img_again = F_t.vflip(vflipped_img)
+
+		assert vflipped_img.shape == img_tensor.shape


Can we use self.assertEqual here? I'd prefer to move away from the raw asserts and use self.assert* methods from unittest for newer test

fmassa · 2019-10-15T14:12:36Z

test/test_functional_tensor.py

+		vflipped_img_again = F_t.vflip(vflipped_img)
+
+		assert vflipped_img.shape == img_tensor.shape
+		assert torch.equal(img_tensor, vflipped_img_again)


I think I'd prefer using self.assertTrue here.

fmassa · 2019-10-15T14:13:13Z

torchvision/transforms/functional_tensor.py

+    """Vertically flip the given the Image Tensor.
+
+    Args:
+        img_tensor (Tensor): Image Tensor to be flipped.


Can you specify in the documentation what is the expected format of the tensor? Is it HxWxC or CxHxW?

fmassa · 2019-10-15T14:14:12Z

torchvision/transforms/functional_tensor.py

+    if not F._is_tensor_image(img_tensor):
+    	raise TypeError('tensor is not a torch image.')
+
+    return img_tensor.flip(1)


Can you make the offset relative to the end of the dimension?
This way, we could make it also work for batches of images / videos.
So it would be something like

return img_tensor.flip(-2)

fmassa · 2019-10-15T14:14:30Z

torchvision/transforms/functional_tensor.py

+    if not F._is_tensor_image(img_tensor):
+    	raise TypeError('tensor is not a torch image.')
+
+    return img_tensor.flip(2)


Same comment, can you do .flip(-1) here?

surgan12 · 2019-10-15T14:17:36Z

@fmassa , got'em. Making the updates.

codecov-io · 2019-10-15T14:39:07Z

Codecov Report

Merging #1465 into master will decrease coverage by <.01%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master    #1465      +/-   ##
==========================================
- Coverage   64.08%   64.07%   -0.01%     
==========================================
  Files          80       81       +1     
  Lines        6328     6338      +10     
  Branches      973      975       +2     
==========================================
+ Hits         4055     4061       +6     
- Misses       1986     1988       +2     
- Partials      287      289       +2

Impacted Files	Coverage Δ
torchvision/transforms/functional_tensor.py	`60% <60%> (ø)`

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 f16b672...444a69f. Read the comment docs.

fmassa

Lint is still failing, and I have a minor nit but otherwise looks good to be merged, thanks!

I need to figure out what's happening with CircleCI though

fmassa · 2019-10-15T15:18:57Z

test/test_functional_tensor.py

+
+
+if __name__ == '__main__':
+    unittest.main()


linter is failing due to the missing newline here

fmassa · 2019-10-15T15:19:30Z

torchvision/transforms/functional_tensor.py

+    """Vertically flip the given the Image Tensor.
+
+    Args:
+        img_tensor (Tensor): Image Tensor to be flipped in the form CXHXW.


nit: can you make it CxHxW or [C, H, W]? I think it's more readable.

fmassa · 2019-10-15T15:27:29Z

@surgan12 the CircleCI error is very weird, it looks like it's trying to run from your account.

Can you try creating a new branch in your fork of torchvision (with the current commits), and then creating a new PR?

git checkout -b tensor-flip
# assuming that me points to your remote
git push me tensor-flip

and then try again?

surgan12 · 2019-10-15T15:45:46Z

Sure will do that !

surgan12 added 2 commits October 15, 2019 19:36

vflip and hflip tensor

9664ec8

vflip and hflip tensor

4a3946d

fmassa requested changes Oct 15, 2019

View reviewed changes

changes made

444a69f

lint

75d1396

fmassa requested changes Oct 15, 2019

View reviewed changes

surgan12 closed this Oct 15, 2019

This was referenced Oct 17, 2019

Changing all the raw asserts to self.assert in tests #1483

Closed

Make crop scriptable #1379

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making Vflip and Hflip in Tensor format #1465

Making Vflip and Hflip in Tensor format #1465

surgan12 commented Oct 15, 2019 •

edited

Loading

fmassa left a comment

fmassa Oct 15, 2019 •

edited

Loading

fmassa Oct 15, 2019

fmassa Oct 15, 2019

fmassa Oct 15, 2019

fmassa Oct 15, 2019

surgan12 commented Oct 15, 2019

codecov-io commented Oct 15, 2019

fmassa left a comment

fmassa Oct 15, 2019

fmassa Oct 15, 2019

fmassa commented Oct 15, 2019 •

edited

Loading

surgan12 commented Oct 15, 2019

Making Vflip and Hflip in Tensor format #1465

Making Vflip and Hflip in Tensor format #1465

Conversation

surgan12 commented Oct 15, 2019 • edited Loading

fmassa left a comment

Choose a reason for hiding this comment

fmassa Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

fmassa Oct 15, 2019

Choose a reason for hiding this comment

fmassa Oct 15, 2019

Choose a reason for hiding this comment

fmassa Oct 15, 2019

Choose a reason for hiding this comment

fmassa Oct 15, 2019

Choose a reason for hiding this comment

surgan12 commented Oct 15, 2019

codecov-io commented Oct 15, 2019

Codecov Report

fmassa left a comment

Choose a reason for hiding this comment

fmassa Oct 15, 2019

Choose a reason for hiding this comment

fmassa Oct 15, 2019

Choose a reason for hiding this comment

fmassa commented Oct 15, 2019 • edited Loading

surgan12 commented Oct 15, 2019

surgan12 commented Oct 15, 2019 •

edited

Loading

fmassa Oct 15, 2019 •

edited

Loading

fmassa commented Oct 15, 2019 •

edited

Loading