Skip to content

Allow indexing tensors with both CPU and CUDA tensors#5583

Merged
ezyang merged 2 commits intopytorch:masterfrom
zou3519:indexing-types
Mar 7, 2018
Merged

Allow indexing tensors with both CPU and CUDA tensors#5583
ezyang merged 2 commits intopytorch:masterfrom
zou3519:indexing-types

Conversation

@zou3519
Copy link
Copy Markdown
Contributor

@zou3519 zou3519 commented Mar 5, 2018

This copies indices to the same device as src and then performs the indexing.

cc @colesbury

Test Plan

code reading
Unit test (it's a sanity test and probably doesn't hit any edge cases, but I'm not sure what those look like)

Comment thread test/test_cuda.py Outdated
import re
import unittest
from itertools import repeat
import random

This comment was marked as off-topic.

@apaszke
Copy link
Copy Markdown
Contributor

apaszke commented Mar 6, 2018

Do we want to add it to improve compatibility with CPU scalars? I don't think it's a good idea to support cross-device indexing with non-scalar tensors, since we generally disallow such operations (they are very expensive!).

@fmassa
Copy link
Copy Markdown
Member

fmassa commented Mar 6, 2018

@apaszke I think this is fixing a regression, as I believe we used to support indexing CUDA tensors with CPU tensors

@zou3519
Copy link
Copy Markdown
Contributor Author

zou3519 commented Mar 6, 2018

@apaszke the context is that right now the error message for indexing a CUDA tensor with a CPU tensor is wrong: #5559 and I thought it would be easiest to fix by allowing cross-device indexing. If we think this isn't a good idea I can go back and just fix the error message

@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented Mar 6, 2018

@pytorchbot retest this please

@colesbury
Copy link
Copy Markdown
Member

I think this is the right strategy. The indexing operations allow many different types to be used as indices. For example, you can index a CUDA tensor with a Python list or NumPy array. It seems to follow that you should be able to use a PyTorch CPU tensor as well.

@yf225
Copy link
Copy Markdown
Contributor

yf225 commented Mar 6, 2018

@pytorchbot retest this please

@yf225
Copy link
Copy Markdown
Contributor

yf225 commented Mar 7, 2018

@zou3519
Copy link
Copy Markdown
Contributor Author

zou3519 commented Mar 7, 2018

@yf225 I'm taking a look

laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
* Allow indexing tensors with both CPU and CUDA tensors

* Remove stray import
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.

6 participants