detach returned tensors from source tensors in tensor constructors by weiyangfb · Pull Request #11815 · pytorch/pytorch

weiyangfb · 2018-09-18T19:34:29Z

fix PR [Easy] make copy constructed tensor a leaf variable when using torch.tensor(sourceTensor) #11061 by moving detach_() and set_requires_grad() to torch.tensor_ctor() and tensor.new_tensor, and also removed warnings and args_requires_grad from internal_new_from_data
with this patch, the returned tensor from tensor_ctor() and new_tensor will be detached from source tensor, and set requires_grad based on the input args
torch.as_tensor retains its behavior as documented

…w_tensor()

torch/csrc/utils/tensor_new.cpp

  if (r.idx == 0) {
+    PyObject* data = r.pyobject(0);
+    if (THPVariable_Check(data)) {
+      PyErr_WarnEx(PyExc_UserWarning,


test/test_torch.py

@@ -2397,29 +2397,23 @@ def test_tensor_factory(self):
            self.assertEqual(5., res1[0].item())

    def test_tensor_factory_copy_var(self):


…ent dtype and device

facebook-github-bot

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

test/test_torch.py

+        # test torch.as_tensor()
+        source = source.add(1)  # make source non-leaf
+        check_copy(torch.as_tensor(source), source.is_leaf, source.requires_grad)
+        check_copy(torch.as_tensor(source, dtype=torch.float), True, False)


torch/csrc/utils/tensor_new.cpp

-               r.pyobject(0))
-        .set_requires_grad(r.toBool(3));
+               data);
+    new_tensor.detach_(); // making new_tensor a leaf node


torch/csrc/utils/tensor_new.cpp

+    PyObject* data = r.pyobject(0);
+    if (THPVariable_Check(data)) {
+      PyErr_WarnEx(PyExc_UserWarning,
+        "To copy construct from a tensor, it is recommended to use sourceTensor.clone().detach() "


torch/csrc/utils/tensor_new.cpp

  if (r.idx == 0) {
+    PyObject* data = r.pyobject(0);
+    if (THPVariable_Check(data)) {
+      PyErr_WarnEx(PyExc_UserWarning,


torch/csrc/utils/tensor_new.cpp

-               args_requires_grad)
-        .set_requires_grad(r.toBool(3));
+               type_inference);
+    new_tensor.detach_(); // making new_tensor a leaf node


torch/csrc/utils/tensor_new.cpp

  ParsedArgs<3> parsed_args;
  auto r = parser.parse(args, kwargs, parsed_args);
  if (r.idx == 0) {
+    at::optional<Device> device_opt = r.deviceOptional(2);


apaszke

Semantics of torch.as_tensor need to be clarified.

test/test_torch.py

+        def check_copy(copy, copy_is_leaf, copy_requires_grad):
+            self.assertEqual(copy.data, source.data)
+            self.assertTrue(copy.is_leaf == copy_is_leaf)
+            self.assertTrue(copy.requires_grad == copy_requires_grad)


torch/_torch_docs.py

+    :func:`as_tensor()` reads out 'the data' from whatever it is passed,
+    and constructs a leaf variable. Therefore ``tensor.as_tensor(x, dtype=dtype, device=device)``
+    is equivalent to ``x.clone().detach().to(dtype, device)``.
+    The equivalents using ``clone()`` and ``detach()`` are recommended.


torch/csrc/utils/tensor_new.cpp

  if (r.idx == 0) {
+    PyObject* data = r.pyobject(0);
+    if (THPVariable_Check(data)) {
+      PyErr_WarnEx(PyExc_UserWarning,


torch/csrc/utils/tensor_new.cpp

-        typeWithDefault(r, 1, 2, type), r.deviceOptional(2), r.pyobject(0), false, false, type_inference);
+    bool is_copy = false;
+
+    if (THPVariable_Check(data)) {


ssnl · 2018-09-20T20:20:44Z

Currently torch.sparse_coo_tensor(i, v, torch.Size(size)) warns

test_autograd.py:576: UserWarning: To copy construct from a tensor, it is recommended to use sourceTensor.clone().detach(
) or sourceTensor.clone().detach().requires_grad_(True), rather than torch.tensor(sourceTensor).
  x = torch.sparse_coo_tensor(i, v, size)

It doesn't really make sense. Does this patch remove that?

weiyangfb · 2018-09-20T20:44:34Z

@ssnl Yes, it warns because torch.sparse_coo_tensor calls into internal_new_from_data, where I put the warning previously. I removed it at this PR, so it should be gone after this lands.

…ts, modify docs

facebook-github-bot

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

Summary: - fix PR pytorch#11061 by moving `detach_()` and `set_requires_grad()` to `torch.tensor_ctor()` and `tensor.new_tensor`, and also removed warnings and `args_requires_grad` from `internal_new_from_data ` - with this patch, the returned tensor from `tensor_ctor()` and `new_tensor` will be detached from source tensor, and set requires_grad based on the input args - `torch.as_tensor` retains its behavior as documented gchanan apaszke Pull Request resolved: pytorch#11815 Differential Revision: D9932713 Pulled By: weiyangfb fbshipit-source-id: 4290cbc57bd449954faadc597c24169a7b2d8259

weiyangfb requested review from apaszke, colesbury, ezyang, gchanan, soumith and zdevito as code owners September 18, 2018 19:34

weiyangfb force-pushed the create_tensor_requires_grad_false branch from c599748 to d92b7dc Compare September 18, 2018 19:41

fix prevoius incorrect PR by moving detach_() to tensor_ctor() and ne…

9e9fdf3

…w_tensor()

weiyangfb force-pushed the create_tensor_requires_grad_false branch from d92b7dc to 9e9fdf3 Compare September 18, 2018 19:43

only raise warnings for when copy from a tensor

4b680f0

ssnl reviewed Sep 18, 2018

View reviewed changes

test/test_torch.py

@@ -2397,29 +2397,23 @@ def test_tensor_factory(self):

self.assertEqual(5., res1[0].item())

def test_tensor_factory_copy_var(self):

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

add warnings and test for as_tensor when copy from tensor with differ…

d276942

…ent dtype and device

facebook-github-bot reviewed Sep 19, 2018

View reviewed changes

gchanan reviewed Sep 19, 2018

View reviewed changes

driazati mentioned this pull request Sep 19, 2018

[easy] Fix warning about copy when not copying #11871

Closed

apaszke suggested changes Sep 19, 2018

View reviewed changes

keep the tensor returned from as_tensor to be differentiable, add tes…

36d298c

…ts, modify docs

weiyangfb force-pushed the create_tensor_requires_grad_false branch from a211893 to 36d298c Compare September 20, 2018 22:23

facebook-github-bot reviewed Sep 20, 2018

View reviewed changes

apaszke approved these changes Sep 21, 2018

View reviewed changes

soumith changed the title ~~fix PR #11061~~ detach returned tensors from source tensors in tensor constructors Sep 21, 2018

facebook-github-bot closed this in 817e83f Sep 21, 2018

weiyangfb mentioned this pull request Sep 25, 2018

Fix warnings emitted when testing distributions #12038

Closed

ezyang added the merged label Jun 26, 2019

		@@ -2397,29 +2397,23 @@ def test_tensor_factory(self):
		self.assertEqual(5., res1[0].item())

		def test_tensor_factory_copy_var(self):

Conversation

weiyangfb commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

apaszke left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

ssnl commented Sep 20, 2018

Uh oh!

weiyangfb commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

weiyangfb commented Sep 18, 2018 •

edited

Loading

weiyangfb commented Sep 20, 2018 •

edited

Loading