Add `torch.movedim` #41480

kshitij12345 · 2020-07-15T16:40:46Z

#38349 #36048

TODO:

Tests
Docs

dr-ci · 2020-07-15T17:24:11Z

💊 CI failures summary and remediations

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

💚 💚 Looks good so far! There are no failures yet. 💚 💚

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 on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 64 times.

ezyang · 2020-07-16T02:14:49Z

@zou3519 I assigned this to you because I remember seeing you implement a baby version of moveaxis for vmap. LMK if this assignment is wrong.

zou3519 · 2020-07-16T14:01:09Z

Yeah, I'm happy to review this. @kshitij12345 -- can you let me know when this is ready for review, or if you want me to take a look at it now? (github says the PR is current a draft)

kshitij12345 · 2020-07-16T14:13:07Z

@zou3519

I'll ping you once I feel the PR is complete from my side.
Till then feel free to have a look and share any feedback.

Thank You!

zou3519

I skimmed through this pretty fast to put up a few initial comments, but ping me when this is ready @kshitij12345 and I'll provide a full review

aten/src/ATen/native/TensorTransformations.cpp

aten/src/ATen/native/native_functions.yaml

* move code to TensorShape * update error msg * update test to check for updated error msg

kshitij12345 · 2020-07-16T18:32:00Z

@zou3519
Would be great if you can review.
Have added tests. And addressed all other comments except for whether the name should be movedim or moveaxis. I feel it will be better to just alias the name to movedim later. Do let me know how you feel about that.
Thank You!

@mruberry
Would be great if you can have a look as well. Thanks!

test/test_autograd.py

zou3519

I haven't read the test code or the documentation yet but the implementation looks correct to me

aten/src/ATen/native/TensorShape.cpp

mruberry · 2020-07-21T16:52:00Z

torch/_torch_docs.py

+           r"""
+movedim(input, source, destination) -> Tensor
+
+Move `source` dim/s of the :attr:`input` to position determined by `destination`.


Suggestion:

"Moves the dimension(s) of :attr:input at the position(s) in :attr:source to the position(s) in :attr:destination.
Other dimensions of :attr:input that are not explicitly moved remain in their original order and appear at the positions not specified in :attr:destination."

Thanks! I wasn' t sure how to phrase it better.

* pending src -> source, dst -> destination * update doc

kshitij12345 · 2020-07-21T19:24:42Z

@mruberry @zou3519

Have made the required changes.

Please review :)

Thanks!

mruberry · 2020-07-21T19:33:58Z

@mruberry @zou3519

Have made the required changes.

Please review :)

Thanks!

Thanks @kshitij12345, docs look good to me now! I'll let @zou3519 have final say.

zou3519

LGTM

facebook-github-bot

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

zou3519 · 2020-07-21T21:15:11Z

aten/src/ATen/native/TensorShape.cpp

+  auto source_iter = std::remove(source_dims.begin(), source_dims.end(), -1);
+  auto destination_iter = std::remove(destination_dims.begin(), destination_dims.end(), -1);


An internal linter pointed these out. source_iter and destination_iter are never actually used, can we remove them and just do:

std::remove(source_dims.begin(), source_dims.end(), -1); std::remove(destination_dims.begin(), destination_dims.end(), -1);

?

Actually, I thought about it a little more. It would be nice to use source_iter and destination_iter to assert that source_dims and destination_dims have the correct number of elements. So something like

TORCH_INTERNAL_ASSERT(std::distance(source_dims.begin(), source_iter) == rest_dim); TORCH_INTERNAL_ASSERT(std::distance(destination_dims.begin(), destination_iter) == rest_dim);

But either way, we should either use source_iter / destination_iter or delete them

Makes sense.
Will use them as suggested in second comment.

Thanks!

facebook-github-bot

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

zou3519 · 2020-07-23T18:07:46Z

Thanks for the contribution, @kshitij12345! There were many times in the past when I wanted to use this feature and I am glad we finally added it to PyTorch

facebook-github-bot · 2020-07-24T00:21:32Z

@zou3519 merged this pull request in 2666571.

Summary: Noticed this bug in `torch.movedim` (#41480). [`std::unique`](https://en.cppreference.com/w/cpp/algorithm/unique) only guarantees uniqueness for _sorted_ inputs. The current check lets through non-unique values when they aren't adjacent to each other in the list, e.g. `(0, 1, 0)` wouldn't raise an exception and instead the algorithm fails later with an internal assert. Pull Request resolved: #44307 Reviewed By: mrshenli Differential Revision: D23598311 Pulled By: zou3519 fbshipit-source-id: fd6cc43877c42bb243cfa85341c564b6c758a1bf

boeddeker · 2020-11-26T16:40:39Z

I have a question, does movedim do the same as np.moveaxis?
If yes, is there a reason, why it is renamed in torch?
It is annoying to remember the names of torch and numpy.

If I remember correct, many torch functions started to support axis as an alternative to dim.
And since many users are familiar with numpy moveaxis would not confuse users.

kshitij12345 · 2020-11-26T17:10:45Z

@boeddeker Yes, movedim has the same functionality as np.moveaxis.

It was named movedim to be more consistent with torch's scheme of using dim.

The plan was to add an alias for it.
#41480 (comment)
#41480 (comment)

cc: @mruberry

mruberry · 2020-11-26T21:37:48Z

@kshitij12345 is correct and we would accept a PR adding the alias. It just hasn't been done yet.

Summary: Reference: #38349 #36048 #41480 (comment) Pull Request resolved: #48581 Reviewed By: bdhirsh Differential Revision: D25276307 Pulled By: mruberry fbshipit-source-id: 3e3e4df1343c5ce5b71457badc43f08c419ec5c3

add moveaxis impl

47b53e2

pytorchbot added the open source label Jul 15, 2020

kshitij12345 marked this pull request as draft July 15, 2020 16:58

kshitij12345 added 2 commits July 15, 2020 22:38

add docs

2ec66e9

add dim uniqueness check

b15a905

ezyang requested a review from zou3519 July 16, 2020 02:14

ezyang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jul 16, 2020

kshitij12345 added 4 commits July 16, 2020 11:15

remove native namespace qualifier

e73f592

add signature to _overrides

2206c0e

add autograd test

2e58ae6

add test for invalid args

b02c386

zou3519 reviewed Jul 16, 2020

View reviewed changes

mruberry added the module: numpy Related to numpy support, and also numpy compatibility of our operators label Jul 16, 2020

kshitij12345 added 4 commits July 16, 2020 23:14

add test

6eccc99

address comments

75d46c6

* move code to TensorShape * update error msg * update test to check for updated error msg

update overload name

2cc8692

Merge branch 'master' into numpy/develop/moveaxis

88b7209

zou3519 reviewed Jul 16, 2020

View reviewed changes

test/test_autograd.py Outdated Show resolved Hide resolved

zou3519 reviewed Jul 16, 2020

View reviewed changes

kshitij12345 marked this pull request as ready for review July 17, 2020 03:48

kshitij12345 added 5 commits July 17, 2020 14:01

update error msg and corresponding test

92f1ac9

use self_dim variable

f3c0976

use DimVector

5703583

add algorithm walkthrough

66960dc

test_autograd: use device in constructing tensor

345104c

mruberry reviewed Jul 21, 2020

View reviewed changes

kshitij12345 added 2 commits July 21, 2020 23:10

Merge branch 'master' into numpy/develop/moveaxis

7fbd2fc

address changes

9bcd29c

* pending src -> source, dst -> destination * update doc

zou3519 approved these changes Jul 21, 2020

View reviewed changes

facebook-github-bot reviewed Jul 21, 2020

View reviewed changes

zou3519 reviewed Jul 21, 2020

View reviewed changes

add extra asserts after std::remove

3ad19aa

facebook-github-bot reviewed Jul 22, 2020

View reviewed changes

facebook-github-bot closed this in 2666571 Jul 23, 2020

mruberry mentioned this pull request Jul 23, 2020

NumPy-like Functionality Request Rollup #38349

Closed

41 tasks

facebook-github-bot added the merged label Jul 24, 2020

Trinkle23897 mentioned this pull request Jul 24, 2020

Potential Bug thu-ml/tianshou#162

Closed

4 tasks

kshitij12345 deleted the numpy/develop/moveaxis branch July 25, 2020 06:38

peterbell10 mentioned this pull request Sep 8, 2020

Fix uniqueness check in movedim #44307

Closed

ngimel mentioned this pull request Sep 11, 2020

Add moveaxis function #36048

Closed

mruberry added the Merged label Oct 28, 2020

kshitij12345 mentioned this pull request Nov 30, 2020

[numpy] Add torch.moveaxis #48581

Closed

joshuacwnewton mentioned this pull request May 14, 2021

Intervertebral disc labeling using pose estimation ivadomed/ivadomed#787

Closed

7 tasks

		auto source_iter = std::remove(source_dims.begin(), source_dims.end(), -1);
		auto destination_iter = std::remove(destination_dims.begin(), destination_dims.end(), -1);

Add torch.movedim #41480

Add torch.movedim #41480

Uh oh!

Conversation

kshitij12345 commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci bot commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

ezyang commented Jul 16, 2020

Uh oh!

zou3519 commented Jul 16, 2020

Uh oh!

kshitij12345 commented Jul 16, 2020

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!

kshitij12345 commented Jul 16, 2020

Uh oh!

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!

Uh oh!

Uh oh!

mruberry Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

kshitij12345 Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

kshitij12345 commented Jul 21, 2020

Uh oh!

mruberry commented Jul 21, 2020

Uh oh!

zou3519 left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

zou3519 Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

zou3519 Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kshitij12345 Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

zou3519 Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

zou3519 commented Jul 23, 2020

Uh oh!

facebook-github-bot commented Jul 24, 2020

Uh oh!

boeddeker commented Nov 26, 2020

Uh oh!

kshitij12345 commented Nov 26, 2020

Uh oh!

mruberry commented Nov 26, 2020

Uh oh!

Add `torch.movedim` #41480

Add `torch.movedim` #41480

kshitij12345 commented Jul 15, 2020 •

edited

Loading

dr-ci bot commented Jul 15, 2020 •

edited

Loading

zou3519 Jul 21, 2020 •

edited

Loading