[READY] Preserve sparse tensor shape and dim invariants, and add scalar tensor support by yf225 · Pull Request #9279 · pytorch/pytorch

yf225 · 2018-07-09T19:28:32Z

When 0-sized dimension support is added, we expect an empty sparse tensor to be a 1-dimensional tensor of size [0], with sparseDims == 1 and denseDims == 0. Also, we expect the following invariants to be preserved at all times:

_sparseDims + _denseDims = len(shape)
_indices.shape: dimensionality: 2,  shape: (_sparseDims, nnz)
_values.shape:  dimensionality: 1 + _denseDims.  shape: (nnz, shape[_sparseDims:])

This PR fixes various places where the invariants are not strictly enforced when 0-sized dimension support is enabled.

Tested and test_sparse.py passes locally on both CPU and CUDA with the USE_TH_SIZE_ZERO_DIM flag.

aten/src/ATen/SparseTensorImpl.cpp

aten/src/ATen/SparseTensorImpl.h

test/test_sparse.py

aten/src/ATen/SparseTensorImpl.h

aten/src/ATen/SparseTensorImpl.cpp

aten/src/ATen/SparseTensorImpl.h

aten/src/ATen/native/sparse/SparseTensor.cpp

aten/src/ATen/native/sparse/SparseTensorMath.cpp

aten/src/ATen/native/sparse/SparseTensor.cpp

weiyangfb · 2018-07-16T21:27:12Z

should the invariant description be changed also?

pytorch/aten/src/ATen/SparseTensorImpl.h

Lines 11 to 28 in 9ae77cc

    
           // Ideal INVARIANTS: 
        
           // _sparseDims: range [0, len(shape)]; _sparseDims + _denseDims = len(shape) 
        
           // _denseDims : range [0, len(shape)]; _sparseDims + _denseDims = len(shape) 
        
           // _indices.shape: dimensionality: 2,  shape: (_sparseDims, nnz) 
        
           // _values.shape:  dimensionality: 1 + _denseDims.  shape: (nnz, shape[_sparseDims:]) 
        
           // Actual INVARIANT differences: 
        
           // 1) _sparseDims: range [1, len(shape)] (i.e. we don't allow 0 sparse dimensions) 
        
           // 2) when nnz = 0, there is strange behavior because we lack 0-dimensional sparse tensors.  Namely: 
        
           //    dimensionality == 0, _sparseDims == 0, _denseDims == 0, _indices.shape == {0}, _values.shape == {0} 
        
           // 3) For both _indices.shape and _values.shape, the nnz dimension may be larger than nnz 
        
           // 4) For _values.shape, the non-nnz dimensions may be smaller than the corresponding dimension size, e.g. 
        
           //    a shape (2,3) sparse tensor with _sparseDims == 1, may have _values.shape: (nnz, <=2, <=3). 
        
           // The true size of the sparse tensor (e.g., if you called to_dense() 
        
           // on it).  When THTensor merges into TensorImpl, this field 
        
           // should move to the parent class.

Oh, you've changed it already, nvm :)

yf225 · 2018-07-18T21:28:18Z

@gchanan @ezyang On a high level, I made the following changes:

Got rid of set_indices / set_values / set_nnz in SparseTensorImpl.h, and re-route all the call sites that call these functions to use set_indices_and_values_unsafe, which will set indices and values accordingly, set nnz properly based on the dimensions of indices and values, and also perform checks for all invariants. This way _nnz is guaranteed to be the actual number of non-zero elements in the sparse tensor.
Note that we don't use _nnz to do narrowing on the sparse tensor anymore, but instead we let the caller do the narrowing on the underlying indices and values tensors, and then put them into the sparse tensor by calling set_indices_and_values_unsafe.
There is no sparse_raw_resize_ function anymore, instead there is sparse_resize_and_empty_ function which resizes the sparse tensor and also resets the underlying indices and values tensors to empty. The reason we do this is that it's difficult to preserve the dim invariants when we change the sparse tensor size but it doesn't agree with the dims of the existing indices or values (in which case we could resize the data tensors as well, but it's not a safe operation and should be avoided). Instead, we should let the caller decide whether/how they want to keep the data tensors before resizing.
tensor = torch.sparse_coo_tensor([], [], torch.Size([])) is not supported anymore, because it breaks our assumption that a sparse tensor with empty size should have one element in values (i.e. it should be a scalar sparse tensor). If we need to create an empty sparse tensor, we should use torch.sparse.*Tensor() instead.

After zero-dim size is enabled by default, we can search USE_TH_SIZE_ZERO_DIM in the code and remove all code that is marked as legacy.

yf225 · 2018-07-18T22:10:20Z

@pytorchbot retest this please

yf225 · 2018-07-19T02:32:37Z

@pytorchbot retest this please

yf225 · 2018-07-19T16:55:03Z

@pytorchbot retest this please

gchanan · 2018-07-19T17:24:24Z

should we have a no-arg (or at least no indices, values, shape) constructor, e.g. torch.sparse_coo_tensor()? Because constructing a proper one is complicated or requires using torch.sparse.*Tensor(), which is a style we've moved away from for dense tensors.

aten/doc/Tensor.h

aten/src/ATen/SparseTensorImpl.cpp

aten/src/ATen/SparseTensorImpl.h

aten/src/ATen/native/TensorFactories.cpp

yf225 · 2018-07-20T01:08:38Z

@pytorchbot retest this please

yf225 · 2018-07-20T02:54:59Z

@pytorchbot retest this please

yf225 · 2018-07-20T16:50:36Z

@pytorchbot retest this please

facebook-github-bot

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

yf225 · 2018-07-20T19:21:18Z

I restarted the pr/pytorch-linux-xenial-py3-clang5-asan build at https://ci.pytorch.org/jenkins/job/pytorch-builds/job/pytorch-linux-xenial-py3-clang5-asan-trigger/11103/

li-roy · 2018-07-20T21:23:40Z

@gchanan If we add an no-arg constructor to torch.sparse_coo_tensor(), isn't that a bit confusing because torch.tensor() doesn't work?

gchanan · 2018-07-20T21:35:44Z

@li-roy that's a fair point. Perhaps just a size-based one is sufficient, then.

aten/src/ATen/SparseTensorImpl.h

aten/src/ATen/native/sparse/SparseTensor.cpp

aten/src/ATen/native/TensorFactories.cpp

yf225 · 2018-07-22T05:14:18Z

@li-roy @gchanan Size-based constructor is also added in this PR.

test/test_sparse.py

+        x_dense.resize_as_(y_dense)
+        self.assertEqual(x.shape, y.shape)
+        self.assertEqual(x._sparseDims(), y._sparseDims())
+        self.assertEqual(x._denseDims(), y._denseDims())


…_zero_dim

test/test_sparse.py

+            self._test_resize_shape([1, 1], [1, 2, 3], [2, 2, 3],
+                                    [1, 1], [1, 2, 3], [1, 2, 3])
+
+        # 8. Shrink the size of some dense dimensions on a non-empty sparse tensor [Not Supported]


test/test_sparse.py

gchanan

nice!

facebook-github-bot

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

yf225 · 2018-08-22T23:57:53Z

@pytorchbot retest this please

…r support (#9279) Summary: When 0-sized dimension support is added, we expect an empty sparse tensor to be a 1-dimensional tensor of size `[0]`, with `sparseDims == 1` and `denseDims == 0`. Also, we expect the following invariants to be preserved at all times: ``` _sparseDims + _denseDims = len(shape) _indices.shape: dimensionality: 2, shape: (_sparseDims, nnz) _values.shape: dimensionality: 1 + _denseDims. shape: (nnz, shape[_sparseDims:]) ``` This PR fixes various places where the invariants are not strictly enforced when 0-sized dimension support is enabled. Tested and `test_sparse.py` passes locally on both CPU and CUDA with the `USE_TH_SIZE_ZERO_DIM` flag. Pull Request resolved: pytorch/pytorch#9279 Differential Revision: D8936683 Pulled By: yf225 fbshipit-source-id: 12f5cd7f52233d3b26af6edc20b4cdee045bcb5e

…r support (pytorch#9279) Summary: When 0-sized dimension support is added, we expect an empty sparse tensor to be a 1-dimensional tensor of size `[0]`, with `sparseDims == 1` and `denseDims == 0`. Also, we expect the following invariants to be preserved at all times: ``` _sparseDims + _denseDims = len(shape) _indices.shape: dimensionality: 2, shape: (_sparseDims, nnz) _values.shape: dimensionality: 1 + _denseDims. shape: (nnz, shape[_sparseDims:]) ``` This PR fixes various places where the invariants are not strictly enforced when 0-sized dimension support is enabled. Tested and `test_sparse.py` passes locally on both CPU and CUDA with the `USE_TH_SIZE_ZERO_DIM` flag. Pull Request resolved: pytorch#9279 Differential Revision: D8936683 Pulled By: yf225 fbshipit-source-id: 12f5cd7f52233d3b26af6edc20b4cdee045bcb5e

yf225 requested review from ezyang and gchanan July 9, 2018 19:28

yf225 requested review from apaszke, colesbury, soumith and zdevito as code owners July 9, 2018 19:28

gchanan reviewed Jul 9, 2018

View reviewed changes

yf225 force-pushed the sparse_zero_dim branch from ed89acb to 4c5e513 Compare July 12, 2018 22:09

yf225 changed the title ~~Preserve sparse tensor shape and dim invariants with 0-sized dim support~~ [READY] Preserve sparse tensor shape and dim invariants with 0-sized dim support Jul 13, 2018

yf225 changed the title ~~[READY] Preserve sparse tensor shape and dim invariants with 0-sized dim support~~ [READY] Preserve sparse tensor shape and dim invariants, and add scalar tensor support Jul 13, 2018

gchanan reviewed Jul 13, 2018

View reviewed changes

yf225 force-pushed the sparse_zero_dim branch from 0a93ea0 to 0f247d3 Compare July 18, 2018 21:00

gchanan reviewed Jul 19, 2018

View reviewed changes

facebook-github-bot reviewed Jul 20, 2018

View reviewed changes

gchanan reviewed Jul 20, 2018

View reviewed changes

weiyangfb mentioned this pull request Jul 21, 2018

The state of sparse Tensors #9674

Open

yf225 force-pushed the sparse_zero_dim branch from 26d31a7 to 79b4359 Compare July 22, 2018 05:12

yf225 force-pushed the sparse_zero_dim branch from 1f444cc to cf1199f Compare August 13, 2018 22:11

Merge branch 'master' into sparse_zero_dim

33e4eed

yf225 commented Aug 13, 2018

View reviewed changes

test/test_sparse.py

x_dense.resize_as_(y_dense)

self.assertEqual(x.shape, y.shape)

self.assertEqual(x._sparseDims(), y._sparseDims())

self.assertEqual(x._denseDims(), y._denseDims())

This comment was marked as off-topic.

Sign in to view

Turn off dense equivalence test for resize

bfa8efd

ailzhang added the ready for review (this tag is deprecated) All PRs are ready for review unless they are draft, WIP, or have undismissed requested changes label Aug 14, 2018

yf225 and others added 3 commits August 15, 2018 14:20

Merge branch 'master' into sparse_zero_dim

66fb9e1

Merge branch 'master' of https://github.com/yf225/pytorch into sparse…

dbe492e

…_zero_dim

Disallow # of dense dim change

1e76a9a

yf225 force-pushed the sparse_zero_dim branch 4 times, most recently from c1dc859 to 54a54df Compare August 20, 2018 22:09

sparse and dense resize equivalence test

8c31c08

yf225 force-pushed the sparse_zero_dim branch from 54a54df to 8c31c08 Compare August 20, 2018 22:26

Skip doc check for sparse_resize_ and sparse_resize_and_clear_

2836b02

yf225 force-pushed the sparse_zero_dim branch from a327943 to 2836b02 Compare August 20, 2018 23:36

Clean up

fb76cab

yf225 force-pushed the sparse_zero_dim branch from b5c3947 to fb76cab Compare August 21, 2018 16:08

yf225 commented Aug 21, 2018

View reviewed changes

test/test_sparse.py

self._test_resize_shape([1, 1], [1, 2, 3], [2, 2, 3],

[1, 1], [1, 2, 3], [1, 2, 3])

# 8. Shrink the size of some dense dimensions on a non-empty sparse tensor [Not Supported]

This comment was marked as off-topic.

Sign in to view

Fix ASAN error

b10e06f

gchanan reviewed Aug 22, 2018

View reviewed changes

test/test_sparse.py Outdated

This comment was marked as off-topic.

Sign in to view

gchanan approved these changes Aug 22, 2018

View reviewed changes

Add shape checking after resize

f7b1f23

facebook-github-bot reviewed Aug 22, 2018

View reviewed changes

facebook-github-bot closed this in b14f2e8 Aug 23, 2018

ezyang added the merged label Jun 26, 2019

Conversation

yf225 commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

weiyangfb commented Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yf225 commented Jul 18, 2018

Uh oh!

yf225 commented Jul 18, 2018

Uh oh!

yf225 commented Jul 19, 2018

Uh oh!

yf225 commented Jul 19, 2018

Uh oh!

gchanan commented Jul 19, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

yf225 commented Jul 9, 2018 •

edited

Loading

weiyangfb commented Jul 16, 2018 •

edited

Loading

yf225 commented Jul 22, 2018 •

edited

Loading