[BC-breaking] Fix bugs in torch::tensor constructor #28523

yf225 · 2019-10-23T17:58:11Z

Stack from ghstack:

[WIP] Support C++ tensor advanced indexing #28524 [WIP] Support C++ tensor advanced indexing
[BC-breaking] Fix bugs in torch::tensor constructor #28523 [BC-breaking] Fix bugs in torch::tensor constructor

New features:

Previously, torch::tensor({true, false, true}) throws "tensor_cpu" not implemented for 'Bool'. After this PR, it produces the correct bool tensor, matching the Python API behavior.
Tensors with zero-size dimensions are now supported, e.g. torch::tensor({{}, {}}) produces a tensor with sizes {2, 0}, matching the Python API behavior.

BC-breaking bug fixes and changes:

Previously, torch::tensor({{1}, {2}}) produces a tensor of sizes {2}. After this PR, it produces a tensor of sizes {2, 1}, matching the Python API behavior.
Fixed semantics of torch::tensor(1.1): it now returns a 0-dim tensor instead of a 1-dim tensor, matching the Python API behavior.
After this PR, passing std::initializer_list (NOT braced-init-list) to torch::tensor doesn't work anymore, and the user should pass the equivalent braced-init-list to torch::tensor instead. For example, torch::tensor(std::initializer_list<double>({1.1, 1.2})) doesn't work anymore, and the user should do torch::tensor({1.1, 1.2}). (The reason for this change is that TensorDataContainer cannot have two constructor that takes std::initializer_list, otherwise a value such as {1.1, 1.2} would be ambiguous - it can take the std::initializer_list<TensorDataContainer> constructor, or it can take the std::initializer_list<double> constructor.)
Previously, when passed a non-dtype TensorOptions to the torch::tensor constructor, it always produces a tensor of dtype float. After this PR, it produces tensor of different dtypes based on the dtype of the braced-init-list, matching the behavior of the no-options case.

// Previously:
torch::tensor({1, 2, 3}, torch::TensorOptions(/*non-dtype-options*/)).dtype() -> float
torch::tensor({{1, 2, 3}}, torch::TensorOptions(/*non-dtype-options*/)).dtype() -> float
torch::tensor({1., 2., 3.}, torch::TensorOptions(/*non-dtype-options*/)).dtype() -> float
torch::tensor({{1., 2., 3.}}, torch::TensorOptions(/*non-dtype-options*/)).dtype() -> float

// Now:
torch::tensor({1, 2, 3}, torch::TensorOptions(/*non-dtype-options*/)).dtype() -> int
torch::tensor({{1, 2, 3}}, torch::TensorOptions(/*non-dtype-options*/)).dtype() -> int
torch::tensor({1., 2., 3.}, torch::TensorOptions(/*non-dtype-options*/)).dtype() -> double
torch::tensor({{1., 2., 3.}}, torch::TensorOptions(/*non-dtype-options*/)).dtype() -> double

// As comparison, currently:
torch::tensor({1, 2, 3}).dtype() -> int
torch::tensor({{1, 2, 3}}).dtype() -> int
torch::tensor({1., 2., 3.}).dtype() -> double
torch::tensor({{1., 2., 3.}}).dtype() -> double

Notes:

From now on, the behavior of at::tensor(scalar_value) (which produces a 1-dim tensor) would be different from torch::tensor(scalar_value) (which produces a 0-dim tensor). I will fix the behavior of at::tensor(scalar_value) in a follow-up PR.
From now on, the behavior of at::tensor({1, 2, 3}, torch::TensorOptions(/*non-dtype-options*/)) (which produces a float tensor) would be different from torch::tensor({1, 2, 3}, torch::TensorOptions(/*non-dtype-options*/)) (which produces a an int tensor). I will fix this behavior of at::tensor constructor in a follow-up PR.

Context for the changes in this PR:

The motivation comes from fixing the "torch::tensor({{1}, {2}}) gives tensor of wrong sizes" bug - in order to fix it, I have to move the handling of at::ArrayRef and std::vector into InitListTensor (see below on why we need to do this) and renamed InitListTensor to TensorDataContainer. After such changes, support for bool values comes out of the box without extra effort, and support for tensors with zero-size dimensions only requires adding a default constructor for TensorDataContainer, so I added those two in this PR.

For the semantic change of torch::tensor(1.1), it's actually more effort to preserve the original wrong behavior (i.e. we need to check the sizes of the tensor converted from TensorDataContainer and reshape any scalar tensor to a 1-D tensor). I think preserving the original wrong behavior doesn't give us much value, and since the above changes naturally fix the problem, we should just start using the right behavior instead.

For the "constructor with non-dtype options behavior" fix, the code looks simpler and easier to reason about with the fix, so I included it in this PR.

Why we need to move the handling of at::ArrayRef and std::vector into TensorDataContainer:

torch::tensor({{1}, {2}}) can match this function overload:
torch::tensor(at::ArrayRef<int> values), because {1} and {2} can be treated as
a list-initialization of an int value. However, this will produce a Tensor with sizes {2},
but we actually want a Tensor with sizes {2, 1}. In order to avoid matching this function overload,
we removed the function overload and moved the ability to convert at::ArrayRef<T>
(and similarly std::vector<T>) into TensorDataContainer, and since for braced-init-list the
TensorDataContainer(std::initializer_list<TensorDataContainer>) constructor is always preferred over all other constructors, it will take the std::initializer_list path, and all is good.

Differential Revision: D18234625

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

tools/autograd/templates/variable_factories.h

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

zou3519 · 2019-10-25T18:19:58Z

Could you walk through how in the original code, torch::tensor({{1}, {2}}) produced a tensor of size {2}?

zou3519 · 2019-10-25T18:32:04Z

Could you walk through how in the original code, torch::tensor({{1}, {2}}) produced a tensor of size {2}?

This is what I think was what was going on in the previous code (please let me know if this is right or not):

{1} becomes a InitListTensor of scalar 1
{2} becomes an InitListTensor of scalar 2
{ InitListTensor(1), InitListTensor(2) } becomes an InitListTensor with sizes [2]

zou3519

Still reading, have some initial questions

tools/autograd/templates/variable_factories.h

New features: 1. Previously, `torch::tensor({true, false, true})` throws `"tensor_cpu" not implemented for 'Bool'`. After this PR, it produces the correct bool tensor, matching the Python API behavior. 2. Tensors with zero-size dimensions are now supported, e.g. `torch::tensor({{}, {}})` produces a tensor with sizes `{2, 0}`, matching the Python API behavior. Bug fixes: 1. Previously, `torch::tensor({{1}, {2}})` produces a tensor of sizes `{2}`. After this PR, it produces a tensor of sizes `{2, 1}`, matching the Python API behavior. 2. Fixed semantics of `torch::tensor(1.1)`: it now returns a 0-dim tensor instead of a 1-dim tensor, matching the Python API behavior. BC-breaking changes: 1. `torch::tensor(1.1)` now returns a 0-dim tensor instead of a 1-dim tensor, matching the Python API behavior. Notes: 1. From now on, the behavior of `at::tensor(scalar_value)` (which produces a 1-dim tensor) would be different from `torch::tensor(scalar_value)` (which produces a 0-dim tensor). I will fix the behavior of `at::tensor(scalar_value)` in the next PR. The motivation comes from fixing the "`torch::tensor({{1}, {2}})` gives tensor of wrong sizes" bug - in order to fix it, I have to create the templated `TensorDataContainer` class and move the handling of `at::ArrayRef` and `std::vector` into it. After such changes, support for bool values comes out of the box without extra effort, and support for tensors with zero-size dimensions only requires adding a default constructor for `TensorDataContainer`, so I added those two in this PR. For the semantic change of `torch::tensor(1.1)`, it's actually more effort to preserve the original wrong behavior (i.e. we need to explicitly instantiate `TensorDataContainer<1>` and let its scalar constructor create a 1-D tensor). I think preserving the original wrong behavior doesn't give us much value, and since the above changes naturally fix the problem, we should just start using the right behavior instead. [ghstack-poisoned]

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

yf225 · 2019-10-28T21:24:03Z

Could you walk through how in the original code, torch::tensor({{1}, {2}}) produced a tensor of size {2}?

This is what I think was what was going on in the previous code (please let me know if this is right or not):

{1} becomes a InitListTensor of scalar 1
{2} becomes an InitListTensor of scalar 2
{ InitListTensor(1), InitListTensor(2) } becomes an InitListTensor with sizes [2]

I made a mistake in my original assessment, and here is what actually happened instead:

torch::tensor({{1}, {2}}) matches a previously existing function overload: torch::tensor(at::ArrayRef<int> values), because the compiler looks at {1} and {2} as
list-initializations of int values, and treat torch::tensor({{1}, {2}}) the same as torch::tensor({1, 2}), which produces a Tensor with sizes {2}.

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

yf225 · 2019-10-28T22:04:05Z

test/cpp/api/autograd.cpp

-  auto a = MyFunction::apply(torch::tensor(6, torch::requires_grad()));
-  auto b = Reenter::apply(torch::tensor(9, torch::requires_grad()));
+  auto a = MyFunction::apply(torch::tensor({6}, torch::dtype(torch::kFloat).requires_grad(true)));
+  auto b = Reenter::apply(torch::tensor({9}, torch::dtype(torch::kFloat).requires_grad(true)));


I changed these tensors from 0-D to 1-D and explicitly passed the dtype, so that the original behavior of the tests are preserved.

yf225 · 2019-10-28T22:04:29Z

test/cpp/api/dataloader.cpp

-    return {torch::tensor(static_cast<int64_t>(index)),
-            torch::tensor(static_cast<int64_t>(index))};
+    return {torch::tensor({static_cast<int64_t>(index)}),
+            torch::tensor({static_cast<int64_t>(index)})};


I changed these tensors from 0-D to 1-D, so that the original behavior of the tests are preserved.

yf225 · 2019-10-28T22:06:24Z

test/cpp/api/functional.cpp


 TEST_F(FunctionalTest, SoftMarginLossDefaultOptions) {
-  auto input = torch::tensor({2., 4., 1., 3.}, torch::requires_grad());
+  auto input = torch::tensor({2., 4., 1., 3.}, torch::dtype(torch::kFloat).requires_grad(true));


I explicitly passed the dtype to all torch::tensor calls that use options, so that the resulting tensor's dtype is always what we expect it to be.

yf225 · 2019-10-28T22:06:52Z

test/cpp/api/parallel.cpp

    void reset() override {}
    torch::Tensor forward(torch::Tensor input) {
-      return torch::tensor(input.device().index());
+      return torch::tensor({input.device().index()});


I changed these tensors from 0-D to 1-D, so that the original behavior of the tests are preserved.

zou3519 · 2019-10-30T19:01:16Z

torch/csrc/api/include/torch/detail/TensorDataContainer.h

+              at::kHalf,
+              scalar_type_,
+              "TensorDataContainer_pretty_print_tensor_item", [&] {
+            stream << tensor_[i].item<scalar_t>();


Does stream << tensor_[i].item() work without dispatching?

item() requires a template type T so I think we will need to pass a scalar_t. I also tried stream << tensor_[i] but that prints

1.1 [ CPUFloatType{} ]

instead of 1.1, which is probably not nice for the pretty print.

test/cpp/api/tensor.cpp

zou3519

The approach looks correct to me. I had some minor comments on cosmetics and testing

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

yf225 · 2019-10-30T20:51:35Z

@zou3519 Thanks so much for the reviews! I address all of them and please feel free to look at it again when you have time :D

test/cpp/api/tensor.cpp

torch/csrc/api/include/torch/detail/TensorDataContainer.h

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

kostmo · 2019-10-31T06:09:59Z

CircleCI build failures summary

As of commit ef6898b:

0/2 flaky

Here are the reasons each build failed.

This comment was automatically generated by Dr. CI.
Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

facebook-github-bot · 2019-11-01T05:43:37Z

@yf225 merged this pull request in 595209b.

Fix bugs in torch::tensor constructor

59b63f4

yf225 requested review from ebetica and goldsborough as code owners October 23, 2019 17:58

yf225 mentioned this pull request Oct 23, 2019

[WIP] Support C++ tensor advanced indexing #28524

Closed

yf225 added the module: bc-breaking Related to a BC-breaking change label Oct 23, 2019

yf225 changed the title ~~Fix bugs in torch::tensor constructor~~ [BC-breaking] Fix bugs in torch::tensor constructor Oct 23, 2019

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

3fdccb0

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

yf225 removed request for ebetica and goldsborough October 23, 2019 18:26

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

0f89dd8

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

yf225 requested a review from zou3519 October 23, 2019 20:48

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

e707bc1

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

yf225 commented Oct 25, 2019

View reviewed changes

tools/autograd/templates/variable_factories.h Outdated Show resolved Hide resolved

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

23c3d76

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

zou3519 reviewed Oct 25, 2019

View reviewed changes

yf225 added 7 commits October 26, 2019 01:15

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

00675a9

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

f68787f

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

d7ad2f8

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

da243d6

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

47ee971

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

2b05849

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

yf225 mentioned this pull request Oct 28, 2019

[C++ API parity] Smooth L1 loss #27661

Closed

yf225 added the module: cpp Related to C++ API label Oct 28, 2019

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

1ee8449

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

yf225 requested a review from zou3519 October 28, 2019 22:01

yf225 commented Oct 28, 2019

View reviewed changes

zou3519 reviewed Oct 30, 2019

View reviewed changes

test/cpp/api/tensor.cpp Outdated Show resolved Hide resolved

zou3519 reviewed Oct 30, 2019

View reviewed changes

test/cpp/api/tensor.cpp Outdated Show resolved Hide resolved

zou3519 reviewed Oct 30, 2019

View reviewed changes

test/cpp/api/tensor.cpp Show resolved Hide resolved

zou3519 reviewed Oct 30, 2019

View reviewed changes

Will Feng added 4 commits October 30, 2019 16:07

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

390a168

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

d52b5be

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

108ee5d

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

dc76396

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

zou3519 approved these changes Oct 30, 2019

View reviewed changes

Will Feng added 2 commits October 30, 2019 17:54

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

2cd8124

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

3b1dc95

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

This was referenced Oct 30, 2019

torch::tensor(scalar) behaves differently from at::tensor(scalar) #28929

Open

Add support for multidimensional input to at::tensor #28930

Open

Will Feng added 6 commits October 30, 2019 18:43

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

624af97

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

8c53296

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

0f0707f

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

5e62d08

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

8ced295

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

Update on "[BC-breaking] Fix bugs in torch::tensor constructor"

ef6898b

Fix bugs in torch::tensor constructor gh-metadata: pytorch pytorch 28523 gh/yf225/16/head

yf225 force-pushed the gh/yf225/16/head branch from 9bc36f9 to ef6898b Compare October 31, 2019 16:30

facebook-github-bot closed this in 595209b Oct 31, 2019

facebook-github-bot added the merged label Nov 1, 2019

facebook-github-bot deleted the gh/yf225/16/head branch November 4, 2019 15:15

mruberry added the Merged label Oct 28, 2020

[BC-breaking] Fix bugs in torch::tensor constructor #28523

[BC-breaking] Fix bugs in torch::tensor constructor #28523

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

zou3519 commented Oct 25, 2019

Uh oh!

zou3519 commented Oct 25, 2019

Uh oh!

zou3519 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

yf225 Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

zou3519 Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zou3519 left a comment

Choose a reason for hiding this comment

Uh oh!

yf225 commented Oct 30, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kostmo commented Oct 31, 2019

CircleCI build failures summary

Uh oh!

facebook-github-bot commented Nov 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

yf225 commented Oct 23, 2019 •

edited

Loading

yf225 commented Oct 28, 2019 •

edited

Loading