Skip to content

Add Tensor.T attribute to reverse dimensions #20598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
umanwizard wants to merge 6 commits into from
Closed

Add Tensor.T attribute to reverse dimensions #20598

umanwizard wants to merge 6 commits into from

Conversation

@umanwizard
Copy link
Contributor

@umanwizard umanwizard commented May 16, 2019

For compatibility with numpy

@pytorchbot pytorchbot added module: autograd Related to torch.autograd, and the autograd engine in general module: docs Related to our documentation, both in docs/ and docblocks module: operators module: pybind Related to our Python bindings / interactions with other Python libraries labels May 16, 2019
@umanwizard
Copy link
Contributor Author

umanwizard commented May 16, 2019

cc @ssnl

@umanwizard umanwizard requested a review from fmassa May 16, 2019 17:16
@umanwizard umanwizard added module: numpy Related to numpy support, and also numpy compatibility of our operators and removed module: autograd Related to torch.autograd, and the autograd engine in general module: docs Related to our documentation, both in docs/ and docblocks module: operators module: pybind Related to our Python bindings / interactions with other Python libraries labels May 16, 2019
@li-roy li-roy added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 16, 2019
@umanwizard
Copy link
Contributor Author

umanwizard commented May 24, 2019

Ping -- can someone review this?

fmassa
fmassa reviewed May 24, 2019
View reviewed changes
@pytorchbot pytorchbot added module: autograd Related to torch.autograd, and the autograd engine in general module: docs Related to our documentation, both in docs/ and docblocks module: operators module: pybind Related to our Python bindings / interactions with other Python libraries labels May 24, 2019
@umanwizard umanwizard closed this May 24, 2019
@umanwizard
Copy link
Contributor Author

umanwizard commented May 24, 2019

Wtf. I screwed something up -- didn't mean to close this.

@umanwizard umanwizard reopened this May 24, 2019
facebook-github-bot
facebook-github-bot reviewed May 24, 2019
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.

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

@umanwizard umanwizard requested a review from fmassa May 24, 2019 19:02
gchanan
gchanan reviewed May 24, 2019
View reviewed changes
torch/csrc/autograd/python_variable.cpp
typedef PyObject *(*getter)(PyObject *, void *);
typedef int (*setter)(PyObject *, PyObject *, void *);

PyObject *THPVariable_get_T(THPVariable *self)
Copy link
Contributor

@gchanan gchanan May 24, 2019

Choose a reason for hiding this comment

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

We don't have the ability to do this in C++, yet? Can we add support for it?

Copy link
Contributor Author

@umanwizard umanwizard May 24, 2019

Choose a reason for hiding this comment

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

Sure, what would you like to call it? reverse_dims ? Or just T?

Copy link
Member

@colesbury colesbury May 24, 2019

Choose a reason for hiding this comment

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

I'd suggest T. In general, we've tried to make the C++ Tensor API match the Python API even when it means uncommon names.

facebook-github-bot
facebook-github-bot reviewed May 24, 2019
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.

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

@pytorchbot pytorchbot added the module: internals Related to internal abstractions in c10 and ATen label May 24, 2019
facebook-github-bot
facebook-github-bot reviewed May 24, 2019
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.

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

Brennan Vincent added 2 commits May 28, 2019 14:42
facebook-github-bot
facebook-github-bot reviewed May 28, 2019
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.

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

zdevito pushed a commit to zdevito/ATen that referenced this pull request May 29, 2019
@facebook-github-bot
Add Tensor.T attribute to reverse dimensions (#20598)
73dcd8f 
Summary:
For compatibility with numpy
Pull Request resolved: pytorch/pytorch#20598

Differential Revision: D15499749

Pulled By: umanwizard

fbshipit-source-id: f3306b496667f20169e9b28db3150d12183703bc
@ezyang
Copy link
Contributor

ezyang commented May 29, 2019

This broke Windows:

20:36:33 ======================================================================
20:36:33 FAIL: test_inplace_view_saved_output (__main__.TestAutograd)
20:36:33 ----------------------------------------------------------------------
20:36:33 Traceback (most recent call last):
20:36:33   File "test_autograd.py", line 2817, in test_inplace_view_saved_output
20:36:33     self.assertEqual(dealloc[0], 1)
20:36:33   File "C:\Jenkins\workspace\pytorch-builds\pytorch-win-ws2016-cuda9-cudnn7-py3-test2\test\common_utils.py", line 528, in assertEqual
20:36:33     super(TestCase, self).assertLessEqual(abs(x - y), prec, message)
20:36:33 AssertionError: 1 not less than or equal to 1e-05 : 
20:36:33 
20:36:33 ----------------------------------------------------------------------

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 29, 2019

@umanwizard merged this pull request in 9294de8.

@kostmo
Copy link
Member

kostmo commented May 29, 2019

This PR was reverted due to breakage of test_autogradon pytorch-win-ws2016-cuda9-cudnn7-py3-test2.

@umanwizard umanwizard mentioned this pull request May 30, 2019
Closed
facebook-github-bot pushed a commit that referenced this pull request Jun 5, 2019
@facebook-github-bot
Re-add Tensor.T (#21175)
e268fc9 
Summary:
Something flaky is going on with `test_inplace_view_saved_output` on Windows.

With my PR #20598 applied, the test fails, even though there is no obvious reason it should be related, so the PR was reverted.

Based on commenting out various parts of my change and re-building, I think the problem is with the name -- renaming everything from `T` to `asdf` seems to make the test stop failing. I can't be sure that this is actually the case though, since I could just be seeing patterns in non-deterministic build output...

I spoke with colesbury offline and we agreed that it is okay to just disable this test on Windows for now and not block landing the main change. He will look into why it is failing.

**Test Plan:** I will wait to make sure the Windows CI suite passes before landing this.
Pull Request resolved: #21175

Differential Revision: D15566970

Pulled By: umanwizard

fbshipit-source-id: edf223375d41faaab0a3a14dca50841f08030da3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gchanan gchanan gchanan left review comments

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

@fmassa fmassa fmassa approved these changes

@colesbury colesbury Awaiting requested review from colesbury

@cpuhrsch cpuhrsch Awaiting requested review from cpuhrsch

Assignees

No one assigned

Labels

Merged module: autograd Related to torch.autograd, and the autograd engine in general module: docs Related to our documentation, both in docs/ and docblocks module: internals Related to internal abstractions in c10 and ATen module: numpy Related to numpy support, and also numpy compatibility of our operators module: pybind Related to our Python bindings / interactions with other Python libraries triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@umanwizard @ezyang @facebook-github-bot @kostmo @colesbury @gchanan @fmassa @li-roy @pytorchbot @mruberry