Skip to content

Conversation

@zou3519
Copy link
Contributor

@zou3519 zou3519 commented May 10, 2018

Right now, if we add a zero-filled sparse tensor with another sparse
tensor, both tensors must have the same "density" (dimI, dimV) and size
(tensor.size()) for them to be added successfully. This relaxes that
constraint so that if both tensors have the same tensor.size() and at
least one is zero-filled, they can be added successfully.

Before:

i = torch.LongTensor([[0, 1, 1], [2, 0, 2]])
v = torch.FloatTensor([3, 4, 5]).unsqueeze(1)
sparse_mat = torch.sparse.FloatTensor(i, v, torch.Size([2,3,1]))
zeros = torch.zeros(sparse_mat.size(), layout=torch.sparse_coo)
sparse_mat + zeros

RuntimeError: cadd operands have
incompatible sizes or dimension types
at
../src/THS/generic/THSTensorMath.c:126

After: no error.

@apaszke
Copy link
Contributor

apaszke commented May 11, 2018

I think this patch leads to a pretty important question: Should our sparse library always treat the number of dense and sparse dimensions as an implementation detail, and allow automatic conversions between the types, or should they have hard checks for mismatches (so they effectively become part of the tensor type in a way). I'd slightly lean towards the first option, but it would be good if we pursued it not only in the all zero case, but in all situations (not necessary as this PR, but we should plan that). Otherwise, it will be really inconsistent with the rest of the library.

@zou3519
Copy link
Contributor Author

zou3519 commented May 11, 2018

I agree with your point @apaszke that if we go through with this, we should plan for it in the future. I'm not sure which approach (automatically allowing conversion between types, or treating them separately) is better.

There are some operations (like transpose) for which allowing automatic conversion between types might not be desirable. Let's say we have a sparse tensor with 3 sparse dimensions (dimI) and 3 dense dimensions (dimV). Right now we allow transposing between the sparse dimensions of a tensor, or the dense dimensions of the tensor, but not both

@ezyang
Copy link
Contributor

ezyang commented May 13, 2018

Given that we support dense-sparse operations, this principle extends to varying levels of sparsity. But without a good strategy for actually handling this, you'll be playing whackamole to get this working uniformly everything. Unfortunately, C10 will help with the dense-sparse case, but not more sparse-less sparse case. Maybe the wrapper mechanism we're building might be able to help.

Transpose seems like a different beast altogether.

@zou3519
Copy link
Contributor Author

zou3519 commented May 16, 2018

The context behind this is that there's a bug in nn.Embedding backward (and any backwards that returns sparse gradients) where if a gradient is a zero-filled sparse tensor (from using the padding_idx argument in nn.Embedding), accumulating gradients doesn't work because a zero-filled sparse tensor always has (dimI, dimV) = (tensor.dim(), 0) (See #7469).

@soumith
Copy link
Contributor

soumith commented May 17, 2018

@zou3519 needs rebase, i think, for onnx test to pass

Right now, if we add a zero-filled sparse tensor with another sparse
tensor, both tensors must have the same "density" (dimI, dimV) and size
(tensor.size()) for them to be added successfully. This relaxes that
constraint so that if both tensors have the same tensor.size() and at
least one is zero-filled, they can be added successfully.

Before:
```
i = torch.LongTensor([[0, 1, 1], [2, 0, 2]])
v = torch.FloatTensor([3, 4, 5]).unsqueeze(1)
sparse_mat = torch.sparse.FloatTensor(i, v, torch.Size([2,3,1]))
zeros = torch.zeros(sparse_mat.size(), layout=torch.sparse_coo)
sparse_mat + zeros

RuntimeError: cadd operands have
incompatible sizes or dimension types
at
../src/THS/generic/THSTensorMath.c:126
```

After: no error.
@zou3519 zou3519 force-pushed the sparse-add-zeros branch from 90c9871 to 8373308 Compare May 17, 2018 20:01
@zou3519 zou3519 merged commit 56e7a2c into pytorch:master May 18, 2018
@gchanan
Copy link
Contributor

gchanan commented May 18, 2018

This situation is somewhat similar to contiguity; some operations support both contiguous and non-contiguous inputs, while some only support contiguous inputs (with the obvious difference being that here the property is between different tensors instead of standalone). We could just give a name to this (congruent?) and give error messages to that effect.

weiyangfb pushed a commit to weiyangfb/pytorch that referenced this pull request Jun 11, 2018
Right now, if we add a zero-filled sparse tensor with another sparse
tensor, both tensors must have the same "density" (dimI, dimV) and size
(tensor.size()) for them to be added successfully. This relaxes that
constraint so that if both tensors have the same tensor.size() and at
least one is zero-filled, they can be added successfully.

Before:
```
i = torch.LongTensor([[0, 1, 1], [2, 0, 2]])
v = torch.FloatTensor([3, 4, 5]).unsqueeze(1)
sparse_mat = torch.sparse.FloatTensor(i, v, torch.Size([2,3,1]))
zeros = torch.zeros(sparse_mat.size(), layout=torch.sparse_coo)
sparse_mat + zeros

RuntimeError: cadd operands have
incompatible sizes or dimension types
at
../src/THS/generic/THSTensorMath.c:126
```

After: no error.
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.

5 participants