Fix nonzero for scalars on cuda, to_sparse for scalars on cpu/cuda. #17406
Conversation
I originally set out to fix to_sparse for scalars, which had some overly restrictive checking (sparse_dim > 0, which is impossible for a scalar). This fix uncovered an issue with nonzero: it didn't properly return a size (z, 0) tensor for an input scalar, where z is the number of nonzero elements (i.e. 0 or 1).
There was a problem hiding this comment.
@gchanan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
This current behavior of In [1]: import numpy as np
In [2]: a = np.array(5)
In [3]: a
Out[3]: array(5)
In [4]: a.nonzero()
Out[4]: (array([0]),)
In [5]: a.nonzero()[0]
Out[5]: array([0])
In [6]: b = np.array([5])
In [7]: b.nonzero()[0]
Out[7]: array([0])so we still have the result as expected, while with the current patch scalars don't return anything |
|
@fmassa Yes that is true, but right now the CPU/CUDA code doesn't match and the CUDA code doesn't match the documentation. |
| @@ -284,14 +284,16 @@ SparseTensor dense_to_sparse(const Tensor& self){ | |||
|
|
|||
| SparseTensor dense_to_sparse(const Tensor& self, int64_t sparse_dim){ | |||
| int64_t dims = self.dim(); | |||
| AT_CHECK(sparse_dim > 0, "sparse_dim must be >0"); | |||
| // TODO: it seems like sparse_dim == 0 could be supported even if self.dim() > 0, | |||
| // but this would take some work and doesn't seem particularly useful. | |||
There was a problem hiding this comment.
ISTR myself saying that in this sense, sparse tensors are a strict generalization of dense tensors :)
There was a problem hiding this comment.
Yep, feel free to implement it to make it true!
| Tensor values; | ||
| if (self.dim() > 0) { | ||
| std::vector<Tensor> ix = indices.chunk(indices.size(0), 0); | ||
| values = self.index(ix).squeeze(0).clone(); |
There was a problem hiding this comment.
It's kind of surprising this code doesn't generalize to the self.dim() == 0 case. Where exactly does it fail? index() with an empty vector?
There was a problem hiding this comment.
well even before I don't believe you can chunk empty tensors, and that's consistent with numpy.
There was a problem hiding this comment.
I guess you could go one way or another, but chunking an empty tensor seems like a perfectly well defined thing to do :)
There was a problem hiding this comment.
ya, if that would have fixed the issue I would have looked into it more, but considering neither call worked, it didn't seem worth it.
…#17406) Summary: I originally set out to fix to_sparse for scalars, which had some overly restrictive checking (sparse_dim > 0, which is impossible for a scalar). This fix uncovered an issue with nonzero: it didn't properly return a size (z, 0) tensor for an input scalar, where z is the number of nonzero elements (i.e. 0 or 1). Pull Request resolved: pytorch/pytorch#17406 Differential Revision: D14185393 Pulled By: gchanan fbshipit-source-id: f37a6e1e3773fd9cbf69eeca7fdebb3caa192a19
I originally set out to fix to_sparse for scalars, which had some overly restrictive checking (sparse_dim > 0, which is impossible for a scalar).
This fix uncovered an issue with nonzero: it didn't properly return a size (z, 0) tensor for an input scalar, where z is the number of nonzero elements (i.e. 0 or 1).