[doc] Add note about `torch.flip` returning new tensor and not view. #50041

kshitij12345 · 2021-01-04T14:01:52Z

Reference: #38271

facebook-github-bot · 2021-01-04T14:02:08Z

💊 CI failures summary and remediations

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

3/3 failures possibly* introduced in this PR
- 1/3 non-CircleCI failure(s)

🕵️ 2 new failures recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

pytorch_linux_xenial_py3_clang7_onnx_ort_test2 (1/2)

Step: "Run tests" (full log | diagnosis details | 🔁 rerun)

Error: No such container:path: 0355e9b1a2b4275d7391a7b833a8c57723140f658737c7a56d9fea79b46fe3cc:/var/lib/jenkins/workspace/test/.coverage

Jan 12 15:30:32     onnx.tests.c2_ref_test.TestCaffe2End2End.test_zfnet  time: 50.76 seconds
Jan 12 15:30:32     onnx.tests.onnx_backend_test.OnnxBackendRealModelTest.test_vgg19_cpu  time: 42.52 seconds
Jan 12 15:30:32     operator_test.ctc_beam_search_decoder_op_test.TestCTCBeamSearchDecoderOp.test_ctc_beam_search_decoder  time: 37.78 seconds
Jan 12 15:30:32     onnx.tests.c2_ref_test.TestCaffe2End2End.test_bvlc_reference_caffenet  time: 34.87 seconds
Jan 12 15:30:32     onnx.tests.c2_ref_test.TestCaffe2End2End.test_alexnet  time: 34.57 seconds
Jan 12 15:30:32     onnx.tests.c2_ref_test.TestCaffe2End2End.test_bvlc_reference_rcnn_ilsvrc13  time: 32.55 seconds
Jan 12 15:30:32     operator_test.rnn_cell_test.RNNCellTest.test_multi_lstm  time: 30.75 seconds
Jan 12 15:30:32     memonger_test.MemongerTest.test_simple_memonger  time: 30.03 seconds
Retrieving test reports
Retrieving Python coverage report
Error: No such container:path: 0355e9b1a2b4275d7391a7b833a8c57723140f658737c7a56d9fea79b46fe3cc:/var/lib/jenkins/workspace/test/.coverage


Exited with code exit status 1

pytorch_linux_xenial_py3_clang7_onnx_ort_test1 (2/2)

Step: "Run tests" (full log | diagnosis details | 🔁 rerun)

Error: No such container:path: dfc18305929f9b0d356d19607cda095a0e6691ea0e8f5473ef8cfb6909185c26:/var/lib/jenkins/workspace/test/.coverage

Jan 12 15:31:24     onnx.tests.c2_ref_test.TestCaffe2End2End.test_zfnet  time: 48.95 seconds
Jan 12 15:31:24     onnx.tests.onnx_backend_test.OnnxBackendRealModelTest.test_vgg19_cpu  time: 40.75 seconds
Jan 12 15:31:24     operator_test.ctc_beam_search_decoder_op_test.TestCTCBeamSearchDecoderOp.test_ctc_beam_search_decoder  time: 37.27 seconds
Jan 12 15:31:24     operator_test.rnn_cell_test.RNNCellTest.test_multi_lstm  time: 32.47 seconds
Jan 12 15:31:24     onnx.tests.c2_ref_test.TestCaffe2End2End.test_alexnet  time: 31.63 seconds
Jan 12 15:31:24     onnx.tests.c2_ref_test.TestCaffe2End2End.test_bvlc_reference_rcnn_ilsvrc13  time: 31.28 seconds
Jan 12 15:31:24     onnx.tests.c2_ref_test.TestCaffe2End2End.test_bvlc_reference_caffenet  time: 30.89 seconds
Jan 12 15:31:24     memonger_test.MemongerTest.test_simple_memonger  time: 29.45 seconds
Retrieving test reports
Retrieving Python coverage report
Error: No such container:path: dfc18305929f9b0d356d19607cda095a0e6691ea0e8f5473ef8cfb6909185c26:/var/lib/jenkins/workspace/test/.coverage


Exited with code exit status 1

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

Follow this link to opt-out of these comments for your Pull Requests.

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

This comment has been revised 12 times.

codecov · 2021-01-04T17:22:50Z

Codecov Report

Merging #50041 (ad47b8b) into master (186fe48) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #50041   +/-   ##
=======================================
  Coverage   80.70%   80.70%           
=======================================
  Files        1904     1904           
  Lines      206597   206597           
=======================================
  Hits       166742   166742           
  Misses      39855    39855

mruberry · 2021-01-06T12:33:28Z

torch/_torch_docs.py

@@ -8332,6 +8332,11 @@ def merge_dicts(*dicts):

 Reverse the order of a n-D tensor along given axis in dims.

+.. note::
+    Unlike `np.flip`, `torch.flip` returns a new Tensor.


Let's tweak and simplify this just a bit.

"`torch.flip` makes a copy of :attr:`input`'s data. This is different from `np.flip`, which returns a view."

There are a couple reasons I want to suggest this change:

it starts with torch.flip, no knowledge of NumPy required

the phrase "a new tensor" may be ambiguous; a view appears like "a new tensor," too. We want to be explicit that torch.flip copies the data.

it removes the time complexity discussion. That discussion is correct and relevant, but a little too detailed since it's not the only implication of this behavior. For example, the write semantics also change, but the level of detail of discussing the time complexity suggests that a change like that would be mentioned, too. Best simplify and avoid it.

Then this can be a template to substitute flip{ud, lr} into. Does that sound good to you, @kshitij12345?

The first two points make sense.

I think it would be good to mention the time-complexity as it is a major gotcha #16424 (comment)

OK that's a good point.

Big O notation seems like a lot of machinery to bring to this documentation, though. Could we sneak in the performance discussion with something like (I'm also going to add an explicit reference to NumPy since "np" in the above is not defined):

"This is different from NumPy's np.flip, which quickly returns a view."

If that's too subtle, what about avoiding big O by doing:

"This is different from NumPy's np.flip, which returns a view. Since copying a tensor's data is more work than viewing that data, torch.flip is expected to be slower than np.flip."

I believe flip would be used heavily on CV side in augmentation pipelines, so it might be better to mention about time-complexity.

Big O notation seems like a lot of machinery to bring to this documentation, though.

Agreed.

I would prefer,

"This is different from NumPy's np.flip, which returns a view. Since copying a tensor's data is more work than viewing that data, torch.flip is expected to be slower than np.flip."
as

which quickly returns a view.

Seems a bit ambiguous as to what is quick and what isn't

So change would be

.. note:: This is different from NumPy's np.flip, which returns a view. Since copying a tensor's data is more work than viewing that data, torch.flip is expected to be slower than np.flip.

Just a side note, numpy.flip documentation does mention that it is a constant-time operation. (Maybe we can say torch.flip is linear-time w.r.t number of elements in tensor)

https://numpy.org/doc/stable/reference/generated/numpy.flip.html

".. note:: torch.flip makes a copy of :attr:input's data. This is different from NumPy's np.flip, which returns a view in constant time. Since copying a tensor's data is more work than viewing that data, torch.flip is expected to be slower than np.flip."

Merging your suggestion about constant time + adding back the first sentence. How does this look?

Yup. This feels better. Thanks!

kshitij12345 · 2021-01-11T12:19:39Z

@mruberry PTAL :)

Failure is irrelevant.

mruberry · 2021-01-11T17:00:23Z

torch/_torch_docs.py

@@ -8365,6 +8370,11 @@ def merge_dicts(*dicts):
 Note:
    Equivalent to input[:,::-1]. Requires the array to be at least 2-D.


I'm now noticing these notes. I think both the fliplr and flipud suggestions for advanced in indexing will actually throw errors in PyTorch? Would you check? If so, seems like we should remove these notes?

You are correct indeed.

>>> import torch >>> t = torch.ones((3,2)) >>> t[:, ::-1] Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: step must be greater than zero >>> t[:, ::-1] Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: step must be greater than zero >>> t[::-1, ...] Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: step must be greater than zero >>> t[::1, ...] tensor([[1., 1.], [1., 1.], [1., 1.]])

Updating it to

Note: Requires the array to be at least 2-D.

On second look should we make it,

Note: Requires the tensor to be at least 2-D.

That would be more appropriate. Sure.

mruberry

Cool! Thanks @kshitij12345, and thanks for fixing the old and incorrect notes.

facebook-github-bot

@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-01-13T09:13:54Z

@mruberry merged this pull request in 057be23.

add note in flip, fliplr, flipud docs

f8b19e6

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

pytorchbot added the open source label Jan 4, 2021

kshitij12345 requested a review from mruberry January 5, 2021 09:05

kshitij12345 marked this pull request as ready for review January 5, 2021 09:05

ezyang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jan 5, 2021

mruberry reviewed Jan 6, 2021

View reviewed changes

address comment: update note

fc83ea0

mruberry reviewed Jan 11, 2021

View reviewed changes

kshitij12345 added 2 commits January 11, 2021 22:46

update Note

7f02b2d

Merge branch 'master' into doc/flip/returns-new-tensor

ad47b8b

mruberry approved these changes Jan 12, 2021

View reviewed changes

facebook-github-bot reviewed Jan 12, 2021

View reviewed changes

replace array -> tensor

f6eb4cd

facebook-github-bot reviewed Jan 12, 2021

View reviewed changes

facebook-github-bot closed this in 057be23 Jan 13, 2021

facebook-github-bot added the Merged label Jan 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[doc] Add note about `torch.flip` returning new tensor and not view. #50041

[doc] Add note about `torch.flip` returning new tensor and not view. #50041

kshitij12345 commented Jan 4, 2021

facebook-github-bot commented Jan 4, 2021 •

edited

codecov bot commented Jan 4, 2021 •

edited

mruberry Jan 6, 2021

kshitij12345 Jan 6, 2021

mruberry Jan 6, 2021

kshitij12345 Jan 6, 2021

kshitij12345 Jan 6, 2021

mruberry Jan 6, 2021

kshitij12345 Jan 6, 2021

kshitij12345 commented Jan 11, 2021 •

edited

mruberry Jan 11, 2021

kshitij12345 Jan 11, 2021

kshitij12345 Jan 11, 2021

kshitij12345 Jan 12, 2021

mruberry Jan 12, 2021

mruberry left a comment

facebook-github-bot left a comment

facebook-github-bot left a comment

facebook-github-bot commented Jan 13, 2021

		@@ -8365,6 +8370,11 @@ def merge_dicts(*dicts):
		Note:
		Equivalent to input[:,::-1]. Requires the array to be at least 2-D.

[doc] Add note about torch.flip returning new tensor and not view. #50041

[doc] Add note about torch.flip returning new tensor and not view. #50041

Conversation

kshitij12345 commented Jan 4, 2021

facebook-github-bot commented Jan 4, 2021 • edited

💊 CI failures summary and remediations

🕵️ 2 new failures recognized by patterns

pytorch_linux_xenial_py3_clang7_onnx_ort_test2 (1/2)

pytorch_linux_xenial_py3_clang7_onnx_ort_test1 (2/2)

codecov bot commented Jan 4, 2021 • edited

Codecov Report

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

kshitij12345 commented Jan 11, 2021 • edited

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

mruberry left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 13, 2021

[doc] Add note about `torch.flip` returning new tensor and not view. #50041

[doc] Add note about `torch.flip` returning new tensor and not view. #50041

facebook-github-bot commented Jan 4, 2021 •

edited

codecov bot commented Jan 4, 2021 •

edited

kshitij12345 commented Jan 11, 2021 •

edited