Skip to content

[C++ API] Make torch::Tensor -> at::Tensor#10516

Closed
goldsborough wants to merge 2 commits intopytorch:masterfrom
goldsborough:tensor-is-tensor
Closed

[C++ API] Make torch::Tensor -> at::Tensor#10516
goldsborough wants to merge 2 commits intopytorch:masterfrom
goldsborough:tensor-is-tensor

Conversation

@goldsborough
Copy link
Contributor

@goldsborough goldsborough commented Aug 14, 2018

This PR removes the using Tensor = autograd::Variable; alias from torch/tensor.h, which means torch::Tensor is now at::Tensor. This PR fixes up some last uses of .data() and tidies up the resulting code. For example, I was able to remove TensorListView such that code like

auto loss = torch::stack(torch::TensorListView(policy_loss)).sum() +
    torch::stack(torch::TensorListView(value_loss)).sum();

is now

auto loss = torch::stack(policy_loss).sum() + torch::stack(value_loss).sum();

CC @jgehring

@ebetica

@goldsborough goldsborough changed the title Got rid of more .data() calls [C++ API] Make torch::Tensor -> at::Tensor Aug 14, 2018
facebook-github-bot
facebook-github-bot reviewed Aug 14, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ezyang
Copy link
Contributor

ezyang commented Aug 15, 2018

CC @gchanan

What's going on with the data() method? We may be making moves towards Tensor/Variable unification C++ side earlier rather than later, to avoid a massive perf slowdown when TH starts using Tensor (and previously nonvirtual calls would have become virtual).

@goldsborough
Copy link
Contributor Author

goldsborough commented Aug 15, 2018

@ezyang so it's good that this PR gets rid of .data() usage, no?

@ebetica
Copy link
Contributor

ebetica commented Aug 15, 2018

@ezyang afaik this should only affect the C++ API users. The library writers are free to pass around Variables as they wish. The main issue for the users is that Variable.data() and Tensor.data() do different things.

@gchanan
Copy link
Contributor

gchanan commented Aug 15, 2018

We should rename one of those data functions -- it's too confusing as is.

@goldsborough
Got rid of more .data() calls
e99585e 
torch::Tensor = at::Tensor compiles

Made tests with torch::Tensor = at::Tensor pass

Removed TensorListView
@goldsborough
Add NoGradGuard to Cloneable::clone()
0509855
facebook-github-bot
facebook-github-bot reviewed Aug 16, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goldsborough is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ezyang ezyang added the merged label Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@apaszke apaszke Awaiting requested review from apaszke

@colesbury colesbury Awaiting requested review from colesbury

@ezyang ezyang Awaiting requested review from ezyang

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

@zdevito zdevito Awaiting requested review from zdevito

+1 more reviewer

@ebetica ebetica ebetica approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@goldsborough @ezyang @ebetica @gchanan @facebook-github-bot