Skip to content

Conversation

@li-roy
Copy link
Contributor

@li-roy li-roy commented May 22, 2018

Fixes #7559. Throw an error because we don't support dims of size 0 yet.

This comment was marked as off-topic.

@fmassa
Copy link
Member

fmassa commented May 23, 2018

Hum.... I'm not sure we want to disable this behavior altogether, because in the case where the indexing slice is in the first dimension, it kinda works as is now (returns an empty tensor).

A better fix I think would be to check if the slice size is 0, and returns an empty tensor straight away?

@fmassa
Copy link
Member

fmassa commented May 23, 2018

For example, I think we should have the same behavior for the moment as in the following:

a = torch.rand(3, 3)
print(a[[]])
print(a[:, []])

Both prints an empty tensor, and I think this is the expected behavior for now.

@gchanan
Copy link
Contributor

gchanan commented May 23, 2018

I haven't thought through the implications of @fmassa's comment, but it seems consistent with how we have handled 0-sized things in the past: all sizes that would include a 0 if we supported it are collapsed to (0,).

@li-roy
Copy link
Contributor Author

li-roy commented May 24, 2018

One issue with returning a 0 index tensor is when it's not the last index you specify. For example:

i = slice(0)
x = torch.Tensor(range(8)).view(2, 2, 2)
x[i, 0]
# RuntimeError: dimension out of range (expected to be in range of [-1, 0], but got 1

@fmassa
Copy link
Member

fmassa commented May 24, 2018

Can't we short-circuit indexing if any of the indexing elements is a slice of size zero?

Its quite annoying to have checks everywhere in the code to skip some operations if the result of the operation is an empty tensor, and with the current setup, many things work already.

@li-roy
Copy link
Contributor Author

li-roy commented May 30, 2018

@pytorchbot retest this please

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@ezyang
Copy link
Contributor

ezyang commented Jun 6, 2018

@li-roy Don't forget about this PR!

@li-roy
Copy link
Contributor Author

li-roy commented Jun 8, 2018

@pytorchbot retest this please

@li-roy
Copy link
Contributor Author

li-roy commented Jun 12, 2018

@pytorchbot retest this please

4 similar comments
@li-roy
Copy link
Contributor Author

li-roy commented Jun 13, 2018

@pytorchbot retest this please

@li-roy
Copy link
Contributor Author

li-roy commented Jun 13, 2018

@pytorchbot retest this please

@li-roy
Copy link
Contributor Author

li-roy commented Jun 13, 2018

@pytorchbot retest this please

@li-roy
Copy link
Contributor Author

li-roy commented Jun 14, 2018

@pytorchbot retest this please

Copy link
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

lgtm!

# indexing 0-length slice
self.assertEqual(torch.tensor([]), reference[slice(0)])
self.assertEqual(torch.tensor([]), reference[slice(0), 2])
self.assertEqual(torch.tensor([]), reference[2, slice(0)])

This comment was marked as off-topic.

@zou3519
Copy link
Contributor

zou3519 commented Jun 14, 2018

@pytorchbot retest this please

@soumith soumith merged commit 6869a5f into pytorch:master Jun 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants