Add sparse sum and mean operator #159

kalyc · 2018-08-24T21:28:37Z

Summary

Add sparse support for sum, mean and dot operators

Related Issues

Missing sparse operators

PR Overview

[y] This PR requires new unit tests [y/n] (make sure tests are included)
[n] This PR requires to update the documentation [y/n] (make sure the docs are up-to-date)
[y] This PR is backwards compatible [y/n]
[n] This PR changes the current API [y/n]

roywei

Thanks for the contribution, few comments in-line.

Remove skip if not mxnet backend. these tests can be tested with tf backend as well.
Add assert is_sparse to make sure sparse tensor is used
There is no mxnet sparse implementation in this PR, suggest to change title to Add sparse tests for sum and mean operator. Make it available for tf backend, and make sure tests pass for tf backend. After sparse implementation, we can use these tests to test mxnet backend.

roywei · 2018-08-27T21:28:56Z

tests/keras/backend/backend_test.py

@@ -1702,6 +1702,56 @@ def test_sparse_concat(self):
            assert k_s_d.shape == k_d.shape
            assert_allclose(k_s_d, k_d, atol=1e-05)

+    @pytest.mark.skipif((K.backend() != 'mxnet'),
+                        reason='Testing only for MXNet backend')


There is nothing specific for mxnet, we can test tensorflow backend as well.

the eval() implementation is different for mxnet - eval() converts the array into a numpy array - making it dense & fails the test. In Tensorflow the original class of the sparse tensor doesn't change - https://github.com/tensorflow/tensorflow/blob/r1.10/tensorflow/python/ops/sparse_ops.py#L724

It's the same for both backends, eval() are supposed to convert to dense and return a numpy array. same in tensorflow backend. See tf_backend

With MXNet backend, when we try to evaluate the value of a sparse KerasSymbol - the dense value is returned and because of this is_sparse(eval(KerasSymbol)) is always False - we basically return the numpy array which is always dense.
In TF backend, a SparseTensor class has been used and because of this even when the tensor is converted to dense it always remains an instance of the SparseTensor class

Will this behavior not become an issue during training when weights are sparse tensor?

I dont think this should be an issue. We are just abstracting the _forward_pass() operation in eval() and then converting it to a numpy array.

roywei · 2018-08-27T21:30:43Z

tests/keras/backend/backend_test.py

+
+        W = np.random.random((5, 4))
+
+        k_s = K.eval(K.sum(K.variable(x_sparse), axis=0))


add check for is_sparse like in the test sparse concat

k_s = k.concatenate([k.variable(x_sparse_1), k.variable(x_sparse_2)]) assert k.is_sparse(k_s)

For mxnet backend, currently sparse tensor is not created in K.variable, so this is comparing the same results. Adding assert k.is_sparse() will ensure sparse tensor is used.

added implementation of sparse tensor as well now

roywei · 2018-08-27T21:33:23Z

tests/keras/backend/backend_test.py

+        assert k_s.shape == k_d.shape
+        assert_allclose(k_s, k_d, atol=1e-05)
+
+    @pytest.mark.skipif((K.backend() != 'mxnet'),


same as test_add, There is nothing specific for mxnet, we can test tensorflow backend as well.

see comment above

roywei · 2018-08-27T21:36:22Z

tests/keras/backend/backend_test.py

+        x_sparse = sparse.csr_matrix((x_d, (x_r, x_c)), shape=(4, 5))
+        x_dense = x_sparse.toarray()
+
+        k_s = K.eval(K.mean(K.variable(x_sparse), axis=0))


Same as above, add assert is_sparse

roywei · 2018-08-27T21:36:31Z

tests/keras/backend/backend_test.py

+        x_sparse = sparse.csr_matrix((x_d, (x_r, x_c)), shape=(4, 5))
+        x_dense = x_sparse.toarray()
+
+        k_s = K.eval(K.mean(K.variable(x_sparse)))


Same as above, add assert is_sparse

roywei · 2018-08-27T21:36:51Z

tests/keras/backend/backend_test.py

+        assert k_s.shape == k_d.shape
+        assert_allclose(k_s, k_d, atol=1e-05)
+
+    @pytest.mark.skipif((K.backend() != 'mxnet'),


same as test_add, There is nothing specific for mxnet, we can test tensorflow backend as well.

same as above

roywei

Thanks for the update! Really cool work!
Seems that this is a general solution, not specific to sum and mean ops, could you also enable other sparse tests to verify? for example test_sparse_dot and test_sparse_concat. In addition to adding new test cases, with this implementation, we should enable previously disabled unit tests in Keras that tests sparse.

sandeep-krishnamurthy

Thanks @kalyc . Great work! Looking forward for first end-to-end sparse model :-)

sandeep-krishnamurthy · 2018-08-28T18:19:02Z

keras/backend/mxnet_backend.py

+        return tensor.toarray()
+    elif isinstance(tensor, mx.sym.Symbol):
+        return tensor.stype('default')
+    elif isinstance(tensor, KerasSymbol):


else returning tensor the way it is

sandeep-krishnamurthy · 2018-08-28T18:27:51Z

keras/backend/mxnet_backend.py

@@ -4149,10 +4174,10 @@ def dfs_get_bind_values(node_start):
    return bind_values


-def _keras_variable(name, shape, dtype, is_vector=False, **kwargs):
+def _keras_variable(name, shape, dtype, stype, is_vector=False, **kwargs):


Should we set default to 'default'?

yep will do

sandeep-krishnamurthy · 2018-08-28T18:29:26Z

tests/keras/backend/backend_test.py

@@ -1702,6 +1702,56 @@ def test_sparse_concat(self):
            assert k_s_d.shape == k_d.shape
            assert_allclose(k_s_d, k_d, atol=1e-05)

+    @pytest.mark.skipif((K.backend() != 'mxnet'),
+                        reason='Testing only for MXNet backend')


Will this behavior not become an issue during training when weights are sparse tensor?

sandeep-krishnamurthy · 2018-08-28T18:32:01Z

tests/keras/backend/backend_test.py

@@ -1702,6 +1702,107 @@ def test_sparse_concat(self):
            assert k_s_d.shape == k_d.shape
            assert_allclose(k_s_d, k_d, atol=1e-05)

+    @pytest.mark.skipif((K.backend() != 'mxnet'),
+                        reason='Testing only for MXNet backend')
+    def test_sparse_sum(self):


I think it will be good idea to move all sparse tests to a separate file. may be - tests/keras/backend/sparse_test.py ?
It will be easier later for merge/rebase etc. and also cleanly separates the tests, that can be eventually contributed back to Keras tests

sounds good - will do

kalyc · 2018-08-28T23:44:39Z

PR continued here - #162
Was unable to update this one.

Have enabled sparse dot operator test.
Sparse concat operator would require addition of one more check - will open another PR for it.

sandeep-krishnamurthy · 2018-08-31T00:32:22Z

@kalyc - Please feel free to close this PR as we running through another PR :-)

kalyc · 2018-09-04T19:40:32Z

Closing this PR in place of this one - #162

kalyc requested review from sandeep-krishnamurthy and roywei August 24, 2018 21:28

kalyc changed the title ~~Add sparse sum and mean operator~~ [WIP] Add sparse sum and mean operator Aug 24, 2018

kalyc changed the title ~~[WIP] Add sparse sum and mean operator~~ Add sparse sum and mean operator Aug 24, 2018

roywei suggested changes Aug 27, 2018

View reviewed changes

kalyc added 3 commits August 27, 2018 14:51

Add sparse sum and mean operator

690c97f

Use mx.sym.sum and mx.sym.mean operators API

3ff0da1

Add sparse data handling and more tests for sparse helper functions

bb51b88

kalyc force-pushed the sparse-sum-mean branch from 687c7a1 to bb51b88 Compare August 27, 2018 23:44

Fix PEP-8 style check

e262050

roywei suggested changes Aug 28, 2018

View reviewed changes

sandeep-krishnamurthy reviewed Aug 28, 2018

View reviewed changes

kalyc mentioned this pull request Aug 28, 2018

Add sparse sum, mean and dot operator support #162

Merged

kalyc closed this Sep 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sparse sum and mean operator #159

Add sparse sum and mean operator #159

kalyc commented Aug 24, 2018 •

edited

Loading

roywei left a comment •

edited

Loading

roywei Aug 27, 2018

kalyc Aug 27, 2018

roywei Aug 28, 2018

kalyc Aug 28, 2018

sandeep-krishnamurthy Aug 28, 2018

kalyc Aug 28, 2018

roywei Aug 27, 2018

roywei Aug 27, 2018

kalyc Aug 27, 2018

roywei Aug 27, 2018

kalyc Aug 27, 2018

roywei Aug 27, 2018

kalyc Aug 27, 2018

roywei Aug 27, 2018

kalyc Aug 27, 2018

roywei Aug 27, 2018

kalyc Aug 27, 2018

roywei left a comment

sandeep-krishnamurthy left a comment

sandeep-krishnamurthy Aug 28, 2018

kalyc Aug 28, 2018

sandeep-krishnamurthy Aug 28, 2018

kalyc Aug 28, 2018

sandeep-krishnamurthy Aug 28, 2018

sandeep-krishnamurthy Aug 28, 2018

kalyc Aug 28, 2018

kalyc commented Aug 28, 2018

sandeep-krishnamurthy commented Aug 31, 2018

kalyc commented Sep 4, 2018


		W = np.random.random((5, 4))

		k_s = K.eval(K.sum(K.variable(x_sparse), axis=0))

Add sparse sum and mean operator #159

Add sparse sum and mean operator #159

Conversation

kalyc commented Aug 24, 2018 • edited Loading

Summary

Related Issues

PR Overview

roywei left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roywei left a comment

Choose a reason for hiding this comment

sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalyc commented Aug 28, 2018

sandeep-krishnamurthy commented Aug 31, 2018

kalyc commented Sep 4, 2018

kalyc commented Aug 24, 2018 •

edited

Loading

roywei left a comment •

edited

Loading