fix masked_select for discontiguous outputs #41841

ngimel · 2020-07-22T06:57:17Z

This fixes #41473 for discontiguous input, mask and out. Tests to follow. Reverting #33269 is not a great solution because I'm told masked_select was needed for printing complex tensors.
cc @gchanan , @zou3519, @ezyang

malfet · 2020-07-22T14:38:27Z

aten/src/ATen/native/TensorAdvancedIndexing.cpp

This would be a no-op for contiguous tensors, but would create a copy for non-contiguous ones, right?

And why the same fix does not need to be applied to TensorConfig() created on line 737 for parallel kernel?

According to @zou3519 only serial kernel is affected, and I didn't figure out yesterday how to fix the kernel itself. I'll take a look now.

I root-caused why its serial kernel only, so now I'm sending discontiguous inputs to parallel kernel that can handle them without making additional copies.

gchanan · 2020-07-22T14:57:15Z

why don't we have tests for printing complex tensors? The tests for #41828 pass.

malfet · 2020-07-22T15:08:15Z

why don't we have tests for printing complex tensors? The tests for #41828 pass.
We do, see

pytorch/test/test_torch.py

Lines 3911 to 3921 in fced54a

# test complex tensor

# complex tensor print uses two formatters, one for real values

# and the other for imag values. this is consistent with numpy

x = torch.tensor([2.3 + 4j, 7 + 6j])

self.assertEqual(x.__repr__(), str(x))

self.assertExpectedInline(str(x), '''tensor([2.3000+4.j, 7.0000+6.j])''')

# test scientific notation for complex tensors

x = torch.tensor([1e28 + 2j , -1e-28j])

self.assertEqual(x.__repr__(), str(x))

self.assertExpectedInline(str(x), '''tensor([1.0000e+28+2.0000e+00j, -0.0000e+00-1.0000e-28j])''')

And for strided ones

pytorch/test/test_torch.py

Lines 4052 to 4076 in fced54a

x = torch.ones(100, 2, 2, 10) * (1 + 1j)

y = x.as_strided(size=(100, 2, 10), stride=(2 * 2 * 10, 2 * 10, 1))

self.assertEqual(str(y), y.__repr__())

expected_str = '''\

tensor([[[1.+1.j, 1.+1.j, 1.+1.j, ..., 1.+1.j, 1.+1.j, 1.+1.j],

[1.+1.j, 1.+1.j, 1.+1.j, ..., 1.+1.j, 1.+1.j, 1.+1.j]],

[[1.+1.j, 1.+1.j, 1.+1.j, ..., 1.+1.j, 1.+1.j, 1.+1.j],

[1.+1.j, 1.+1.j, 1.+1.j, ..., 1.+1.j, 1.+1.j, 1.+1.j]],

[[1.+1.j, 1.+1.j, 1.+1.j, ..., 1.+1.j, 1.+1.j, 1.+1.j],

[1.+1.j, 1.+1.j, 1.+1.j, ..., 1.+1.j, 1.+1.j, 1.+1.j]],

...,

[[1.+1.j, 1.+1.j, 1.+1.j, ..., 1.+1.j, 1.+1.j, 1.+1.j],

[1.+1.j, 1.+1.j, 1.+1.j, ..., 1.+1.j, 1.+1.j, 1.+1.j]],

[[1.+1.j, 1.+1.j, 1.+1.j, ..., 1.+1.j, 1.+1.j, 1.+1.j],

[1.+1.j, 1.+1.j, 1.+1.j, ..., 1.+1.j, 1.+1.j, 1.+1.j]],

[[1.+1.j, 1.+1.j, 1.+1.j, ..., 1.+1.j, 1.+1.j, 1.+1.j],

[1.+1.j, 1.+1.j, 1.+1.j, ..., 1.+1.j, 1.+1.j, 1.+1.j]]])\

'''

self.assertExpectedInline(str(y), expected_str)

ngimel · 2020-07-22T15:46:12Z

Ok, maybe we don't need masked_select for complex printing after all, I just remember @anjali411 wanted original PR for something complex-replated.

gchanan · 2020-07-22T16:09:15Z

yep, I checked with @anjali411 and she said we split the real and imaginary parts before printing, so we should be good.

dr-ci · 2020-07-22T16:56:51Z

💊 CI failures summary and remediations

As of commit 535949c (more details on the Dr. CI page):

1/1 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

Failed: pr/caffe2-pytorch-linux-xenial-rocm3.5.1-py3.6-test

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 18 times.

…on CPU (pytorch#33269)" (pytorch#41828)" This reverts commit 71aad6e.

ezyang · 2020-07-22T21:10:49Z

Thanks for the fix. Is malfet doing the full review? Add me if you want me to review too.

ngimel · 2020-07-23T00:26:25Z

@malfet it's ready for review now. Unfortunately it's harder to review, as after revert I have to bring original porting PR back, but now it has a test that explicitly tests for discontiguous mask/value/out, so if that passes, should be good to go.
Unlike before, broadcasted mask/value now will also go through parallel kernel (because they are not contiguous), so the perf would be somewhat worse for those cases. Before broadcasted cases could use serial kernel, because TensorIterator normally does not reorder dimensions in this case. We don't have an API to query TensorIterator if dimensions are reordered, and I did not think it makes sense to bring it just for this case.

facebook-github-bot

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

mruberry

Awesome fix to a couple silent errors!

facebook-github-bot

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

facebook-github-bot · 2020-08-04T02:17:43Z

@ngimel merged this pull request in 7a57088.

malfet reviewed Jul 22, 2020

View reviewed changes

Natalia Gimelshein added 3 commits July 22, 2020 10:30

Revert "Revert "port masked_select from TH to ATen and optimize perf …

8194ab0

…on CPU (pytorch#33269)" (pytorch#41828)" This reverts commit 71aad6e.

fix masked_select for discontiguous outputs

a80fce9

use parallel kernel for potentially permuted inputs

8df3e32

ngimel force-pushed the masked_selec branch from f7c865c to 8df3e32 Compare July 22, 2020 17:32

add test for discontiguous mask_selec

c5ee765

Natalia Gimelshein added 3 commits July 22, 2020 17:28

lint

8a52359

slightly clean up test_masked_select

a45fca8

disable xla test

35fbfef

ailzhang mentioned this pull request Jul 23, 2020

Disable test_masked_select_discontiguous. pytorch/xla#2367

Merged

masked_select test is now disabled on the xla side, reenable it here

535949c

facebook-github-bot reviewed Jul 23, 2020

View reviewed changes

zou3519 mentioned this pull request Jul 27, 2020

'torch.masked_select' behaves differently depending on the device of inputs. #41473

Closed

mruberry approved these changes Aug 3, 2020

View reviewed changes

facebook-github-bot reviewed Aug 3, 2020

View reviewed changes

facebook-github-bot closed this in 7a57088 Aug 4, 2020

facebook-github-bot added the merged label Aug 4, 2020

mruberry added the Merged label Oct 28, 2020

fix masked_select for discontiguous outputs #41841

fix masked_select for discontiguous outputs #41841

Uh oh!

Conversation

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

Uh oh!

Uh oh!

malfet Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

malfet Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

ngimel Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

ngimel Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

gchanan commented Jul 22, 2020

Uh oh!

malfet commented Jul 22, 2020

Uh oh!

ngimel commented Jul 22, 2020

Uh oh!

gchanan commented Jul 22, 2020

Uh oh!

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

Uh oh!

💊 CI failures summary and remediations

ci.pytorch.org: 1 failed

Uh oh!

ezyang commented Jul 22, 2020

Uh oh!

ngimel commented Jul 23, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

mruberry 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!

facebook-github-bot commented Aug 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ngimel commented Jul 22, 2020 •

edited

Loading

dr-ci bot commented Jul 22, 2020 •

edited

Loading