Skip to content

Make randperm work properly on non-contiguous tensors. #23043

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
xuhdev wants to merge 2 commits into from
Closed

Make randperm work properly on non-contiguous tensors. #23043

xuhdev wants to merge 2 commits into from

Conversation

@xuhdev
Copy link
Collaborator

@xuhdev xuhdev commented Jul 18, 2019

Close #22710

@pytorchbot pytorchbot added module: cuda Related to torch.cuda, and CUDA support in general module: operators labels Jul 18, 2019
@xuhdev
Copy link
Collaborator Author

xuhdev commented Jul 18, 2019

The code should be equivalent if the output tensor is contiguous. I wonder if there is a proper way to add a test for non-contiguous tensors...

@xuhdev xuhdev changed the title Make randperm works properly on non-contiguous tensors. [WIP] Make randperm works properly on non-contiguous tensors. Jul 18, 2019
@xuhdev xuhdev force-pushed the op/randperm-cuda branch from f8e6d15 to 54c2fd8 Compare July 18, 2019 22:26
@xuhdev xuhdev changed the title [WIP] Make randperm works properly on non-contiguous tensors. Make randperm works properly on non-contiguous tensors. Jul 18, 2019
@xuhdev xuhdev assigned gchanan and unassigned gchanan Jul 18, 2019
@xuhdev xuhdev requested a review from gchanan July 18, 2019 23:17
@xuhdev xuhdev mentioned this pull request Jul 18, 2019
Closed
@xuhdev xuhdev requested a review from li-roy July 19, 2019 17:59
li-roy
li-roy reviewed Jul 19, 2019
View reviewed changes
Copy link
Contributor

@li-roy li-roy left a comment

Choose a reason for hiding this comment

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

Looks good for the most part. Can you add a test? One way to test noncontiguous is by creating a contiguous tensor, creating a view that's non contiguous (using view, transpose, or anything else).

@xuhdev xuhdev force-pushed the op/randperm-cuda branch 2 times, most recently from b98dc92 to fa9ab89 Compare July 19, 2019 19:25
@xuhdev xuhdev requested a review from li-roy July 19, 2019 19:26
@xuhdev
Copy link
Collaborator Author

xuhdev commented Jul 19, 2019

Looks good for the most part. Can you add a test? One way to test noncontiguous is by creating a contiguous tensor, creating a view that's non contiguous (using view, transpose, or anything else).

OK, I used transpose and add an assertion to guarantee that it has to be non-contiguous (because it seems there is no way I can generate a tensor that is guaranteed to be non-contiguous without looking into the details of implementation).

Another note: The CPU test and CUDA test looks largely the same. I'll probably simplify it in a separate PR.

@cpuhrsch cpuhrsch requested a review from VitalyFedyunin July 19, 2019 19:57
@cpuhrsch cpuhrsch added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jul 19, 2019
@xuhdev xuhdev force-pushed the op/randperm-cuda branch from fa9ab89 to eb9c78e Compare July 19, 2019 20:04
gchanan
gchanan reviewed Jul 22, 2019
View reviewed changes
@xuhdev xuhdev force-pushed the op/randperm-cuda branch from eb9c78e to cb104a3 Compare July 22, 2019 17:53
@xuhdev xuhdev requested a review from gchanan July 22, 2019 17:56
@xuhdev xuhdev force-pushed the op/randperm-cuda branch 2 times, most recently from b4333b8 to 6d4d370 Compare July 22, 2019 23:30
@xuhdev
Copy link
Collaborator Author

xuhdev commented Jul 23, 2019

@pytorchbot rebase this please

VitalyFedyunin
VitalyFedyunin reviewed Jul 23, 2019
View reviewed changes
VitalyFedyunin
VitalyFedyunin suggested changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@VitalyFedyunin VitalyFedyunin left a comment

Choose a reason for hiding this comment

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

Looks good except one tiny thing.

@xuhdev xuhdev force-pushed the op/randperm-cuda branch from ef73f77 to 7b77f24 Compare July 23, 2019 20:08
@xuhdev xuhdev requested a review from VitalyFedyunin July 23, 2019 20:08
facebook-github-bot
facebook-github-bot reviewed Jul 23, 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.

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

@xuhdev xuhdev changed the title Make randperm works properly on non-contiguous tensors. Make randperm work properly on non-contiguous tensors. Jul 23, 2019
gchanan
gchanan approved these changes Jul 24, 2019
View reviewed changes
Copy link
Contributor

@gchanan gchanan left a comment

Choose a reason for hiding this comment

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

thanks!

li-roy
li-roy approved these changes Jul 24, 2019
View reviewed changes
test/test_cuda.py
torch.randperm(small_n, out=res) # No exception expected
self.assertRaises(RuntimeError, lambda: torch.randperm(large_n, out=res))

# Test non-contiguous tensors
Copy link
Contributor

@li-roy li-roy Jul 24, 2019

Choose a reason for hiding this comment

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

thanks!

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 29, 2019

@VitalyFedyunin merged this pull request in 236149e.

zdevito pushed a commit to zdevito/ATen that referenced this pull request Jul 29, 2019
@xuhdev @facebook-github-bot
Make randperm works properly on non-contiguous tensors. (#23043)
6eedcd5 
Summary:
Close pytorch/pytorch#22710
Pull Request resolved: pytorch/pytorch#23043

Differential Revision: D16446340

Pulled By: VitalyFedyunin

fbshipit-source-id: 1760af310fee71b369e1aaaf96546277058611c9
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

@gchanan gchanan gchanan approved these changes

+2 more reviewers

@VitalyFedyunin VitalyFedyunin VitalyFedyunin approved these changes

@li-roy li-roy li-roy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged module: cuda Related to torch.cuda, and CUDA support in general open source 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.

randperm on cuda looks buggy

9 participants

@xuhdev @facebook-github-bot @VitalyFedyunin @gchanan @li-roy @ezyang @cpuhrsch @pytorchbot @mruberry