[MXNET-117] Sparse operator broadcast_mul/div(csr, dense) = csr #10208

haojin2 · 2018-03-22T18:43:40Z

Description

Add a sparse operator on CPU that supports broadcast_mul/div(csr, dense) = csr operations.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-117]
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Add support for broadcast_mul/div(csr, 1Ddense) = csr
Add support for broadcast_mul/div(csr, 2Ddense) = csr
Add test for broadcast_mul/div(csr, 1Ddense) = csr
Add test for broadcast_mul/div(csr, 2Ddense) = csr

Comments

Duplicate PR with #10150, getting a new PR due to renaming of my branch
Example for broadcast_mul/div(csr, 1Ddense) = csr
import mxnet as mx
a = mx.nd.array([[0,0,3],[0,2,0],[1,0,0]]).tostype('csr')
b = mx.nd.array([1,2,3])
mx.nd.broadcast_mul(a,b).asnumpy()
array([[ 0., 0., 3.],
[ 0., 4., 0.],
[ 3., 0., 0.]], dtype=float32)

eric-haibin-lin · 2018-03-23T17:50:33Z

src/operator/tensor/elemwise_binary_broadcast_op.h

+  int& out_stype = out_attrs->at(0);
+  bool dispatched = false;
+  // For GPU, directly fallback
+  if (dev_mask == mshadow::gpu::kDevMask) {


Should dispatch to fcompute for dense inputs/outputs

eric-haibin-lin · 2018-03-23T23:03:24Z

@marcoabreu the pylint result failed on lines not added by this PR. Is there anything not tested on CI?

marcoabreu · 2018-03-26T10:53:18Z

@eric-haibin-lin the pylint check passed in http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-10208/7/pipeline. There was a short time between upgrading the instances and merging my PR to reflect the necessary changes (pylint fixes included) in which pylint failures could have been possible. It should be resolved now.

haojin2 · 2018-03-26T21:16:11Z

@sergeykolychev Hi, my PR is failing some tests because I've changed some interface in Python but I'm not familiar with Perl so I'm not able to make the same changes in Perl. I wonder if you could please give some help on how to make corresponding changes in Perl so that my builds could pass? Thanks!

sergeykolychev · 2018-03-27T01:02:11Z

@haojin2 Hi Hao, certainly, when you need this by the latest ? I'm on vacation right now but if it's pressing I can look at it sooner.

haojin2 · 2018-03-27T01:16:19Z

@sergeykolychev Probably by the end of this month would be a hard deadline for me, since it would be good check this in for the next release. I guess all what we need are some minor changes to the interface similar to my changes to sparse.py in this PR on the Perl side, so I think any help at your earliest convenience is appreciated, thanks for your response and enjoy your vacation!

sergeykolychev · 2018-03-27T01:38:39Z

@haojin2 ok, I'll fix this problem for you until end of Thursday this week.
Don't worry and continue your development, I'll take about the perl side.

haojin2 · 2018-03-27T15:18:17Z

Sure, please take your time! Thanks a lot for your help!

eric-haibin-lin · 2018-03-28T23:00:14Z

tests/python/unittest/test_sparse_operator.py


+@with_seed()
+def test_sparse_broadcast_mul_div():
+    from scipy.sparse import random, csr_matrix


I don't think scipy.sparse.csr_matrix is used here

eric-haibin-lin · 2018-03-28T23:02:41Z

tests/python/unittest/test_sparse_operator.py

+        assert_almost_equal(mx.nd.broadcast_div(mx_lhs, mx_rhs).asnumpy(), np.divide(np_lhs, np_rhs), atol=1e-4)
+    shape = (4,3)
+    np_lhs = random(shape[0], shape[1], density=0.25, dtype=np.float32).tocsr()
+    mx_lhs = mx.nd.sparse.csr_matrix((np_lhs.data, np_lhs.indices, np_lhs.indptr), shape=shape)


Why not just use test_utils.rand_ndarray(shape, stype, density)?

Done, was not aware of that helper function

eric-haibin-lin · 2018-03-28T23:05:22Z

python/mxnet/ndarray/sparse.py

 _set_ndarray_class(_ndarray_cls)


+def add(lhs, rhs):


Need to update the document and clarify that it's equivalent to nd.elemwise_add() when shape matches. Same for other mul, div, sub

eric-haibin-lin · 2018-03-28T23:06:55Z

python/mxnet/ndarray/sparse.py

    >>> mx.nd.sparse.zeros('row_sparse', (1,2), ctx=mx.cpu(), dtype='float16').asnumpy()
    array([[ 0.,  0.]], dtype=float16)
    """
+    # pylint: disable= no-member, protected-access


Is this added to every function? Maybe worth disable it at the beginning of the file instead of disabling it per function.

Not added to every function, so will keep this in this way.

eric-haibin-lin · 2018-03-28T23:08:08Z

src/operator/tensor/elemwise_binary_broadcast_op_basic.cc

                          [ 1.,  1.,  1.]]

+Supported sparse operations:
+   broadcast_mul(csr, dense(1D)) = csr


clarify that this is only supported on CPU

sergeykolychev · 2018-03-29T22:11:24Z

@haojin2 haojin2#1 this fixes the perl tests. Thanks.

haojin2 · 2018-03-29T22:39:11Z

@sergeykolychev Thank you very much!

eric-haibin-lin · 2018-03-29T23:44:51Z

perl-package/AI-MXNet/lib/AI/MXNet/NDArray/Sparse.pm

             '/=' => \&not_implemented;
+
+method add(AI::MXNet::NDArray|Num $other, $reverse=)
+{


I really appreciate your fast response @sergeykolychev thanks for unblocking us. While I'm not a perl user but some documentations for the new function will be great. Maybe worth adding next time

Yeah, I'll add the docs next time with more substantial changes in April. Tried to unblock quickly. Though I don't see anybody using the functions directly and not via the overloaded operators.

eric-haibin-lin · 2018-04-02T20:41:39Z

python/mxnet/ndarray/sparse.py

+
+    Examples
+    --------
+    >>> x = mx.nd.ones((2,3))


I think we should replace the example with the supported case specifically for sparse:

>>> x = mx.nd.ones((2,3)).tostype('csr') >>> y = mx.nd.ones((2,3)).tostype('csr') >>> (x + y).asnumpy() array([[ 2., 2., 2.], [ 2., 2., 2.]], dtype=float32) >>> (x + 2).asnumpy() array([[ 3., 3., 3.], [ 3., 3., 3.]], dtype=float32) >>> mx.nd.sparse.add(x,y).asnumpy() array([[ 2., 2., 2.], [ 2., 2., 2.]], dtype=float32)

eric-haibin-lin · 2018-04-02T20:42:05Z

python/mxnet/ndarray/sparse.py

+    # pylint: enable= no-member, protected-access
+
+
+def multiply(lhs, rhs):


Should these functions be added to __all__ in line 36?

Let's show a specific example for csr multiplied by 1D vector

Changed, please check

eric-haibin-lin · 2018-04-02T20:42:54Z

python/mxnet/ndarray/sparse.py

+
+    Examples
+    --------
+    >>> x = mx.nd.ones((2,3))


Same comment: use sparse ndarray in the example

eric-haibin-lin · 2018-04-02T20:43:36Z

python/mxnet/ndarray/sparse.py

+    >>> (x/y).asnumpy()
+    array([[ 3.,  3.,  3.],
+           [ 3.,  3.,  3.]], dtype=float32)
+    >>> mx.nd.divide(x,y).asnumpy()


Let's show a specific example for csr divided by 1D vector

eric-haibin-lin · 2018-04-02T20:47:30Z

src/operator/tensor/elemwise_binary_broadcast_op_basic.cc


   broadcast_mul(x, y) = [[ 0.,  0.,  0.],
                          [ 1.,  1.,  1.]]



Shall we also edit https://github.com/apache/incubator-mxnet/blob/master/docs/api/python/ndarray/sparse.md#arithmetic-operations and add broadcast_mul / broadcast_div?

eric-haibin-lin · 2018-04-04T00:03:57Z

tests/python/unittest/test_sparse_operator.py

+    def check_broadcast_div(mx_lhs, mx_rhs, np_lhs, np_rhs, dtype):
+        assert_almost_equal(mx.nd.sparse.divide(mx_lhs, mx_rhs).asnumpy(), np.divide(np_lhs, np_rhs), atol=1e-4)
+    stype = 'csr'
+    for num_rows in range(2, 6):


Personally I think this is too much.. This will be tested repeatedly on CI, rand_shape_2d() should be sufficient.

eric-haibin-lin · 2018-04-04T00:07:01Z

tests/python/unittest/test_sparse_operator.py

+    for num_rows in range(2, 6):
+        for num_cols in range(2, 6):
+            shape = (num_rows, num_cols)
+            density = random.uniform(0.15, 0.25)


Actually what about densities = zero and a non-zero? We want to explicitly test the case where density = 0, since that's a special case in your code

eric-haibin-lin · 2018-04-04T00:15:00Z

python/mxnet/ndarray/sparse.py

+    >>> (x+2).asnumpy()
+    array([[ 3.,  3.,  3.],
+           [ 3.,  3.,  3.]], dtype=float32)
+    >>> (x+y).asnumpy()


I think csr + dense will fallback. It's not good to put an example that is not efficient..

eric-haibin-lin · 2018-04-04T00:16:09Z

python/mxnet/ndarray/sparse.py

+    >>> (x*y).asnumpy()
+    array([[ 0.,  0.,  0.],
+           [ 1.,  1.,  1.]], dtype=float32)
+    >>> mx.nd.multiply(x, y).asnumpy()


This should be sparse.multiply, right?

eric-haibin-lin · 2018-04-04T00:16:27Z

python/mxnet/ndarray/sparse.py

+    >>> (x/y).asnumpy()
+    array([[ 6.,  6.,  6.],
+           [ 3.,  3.,  3.]], dtype=float32)
+    >>> mx.nd.divide(x,y).asnumpy()


sparse.divide?

eric-haibin-lin · 2018-04-04T00:16:46Z

python/mxnet/ndarray/sparse.py

+    Examples
+    --------
+    >>> x = mx.nd.ones((2,3)).tostype('csr')
+    >>> y = mx.nd.arange(2).reshape((2,1))


csr - dense will fallback..

This is multiply, subtract has been fixed above

eric-haibin-lin · 2018-04-04T00:18:55Z

src/operator/tensor/elemwise_binary_broadcast_op.h

+  CHECK_EQ(inputs.size(), 2U);
+  CHECK_EQ(outputs.size(), 1U);
+  CHECK_EQ(req.size(), 1U);
+  CHECK_LE(inputs[1].shape().ndim(), 2U) << "input dense matrix should have less than 2 dimensions";


less than -> less than or equal?

eric-haibin-lin · 2018-04-04T00:21:55Z

src/operator/tensor/elemwise_binary_broadcast_op.h

+  } else {
+    if (req[0] != kNullOp) {
+      // broadcast(CSR, Dense(1D)) = CSR
+      if (lhs_stype == kCSRStorage && rhs_stype == kDefaultStorage && out_stype == kCSRStorage) {


What about rhs.shape = (1,1)? Will this work?

Made it work

eric-haibin-lin · 2018-04-04T00:37:27Z

src/operator/tensor/elemwise_binary_broadcast_op.h

+  // If the input is not a vector
+  if ((rhs.shape().ndim() != 1U) && (rhs.shape()[0] != 1) && (rhs.shape()[1] != 1)) {
+    // Currently do not support elementwise_mul/div(csr, dense) = csr, log and exit
+    LogUnimplementedOp(attrs, ctx, inputs, req, outputs);


Let's print a more informational error msg

eric-haibin-lin · 2018-04-04T22:49:11Z

src/operator/tensor/elemwise_binary_broadcast_op.h

+      MSHADOW_IDX_TYPE_SWITCH(output.aux_type(kIdx), CType, {
+        MSHADOW_IDX_TYPE_SWITCH(output.aux_type(kIndPtr), RType, {
+          MXNET_ASSIGN_REQ_SWITCH(req, req_type, {
+            if (dns.shape().ndim() > 1 && dns.shape()[0] == 1 && dns.shape()[1] == 1) {


would rhs = mx.nd.array([5]) work?

eric-haibin-lin · 2018-04-04T23:51:58Z

src/operator/tensor/elemwise_binary_broadcast_op.h

+}
+
+template<typename xpu, typename OP>
+void BinaryBroadcastComputeCsrEx(const nnvm::NodeAttrs& attrs,


rename to BinaryBroadcastComputeEx

eric-haibin-lin · 2018-04-04T23:52:49Z

src/operator/tensor/elemwise_binary_broadcast_op.h

+      if (lhs_stype == kCSRStorage && rhs_stype == kDefaultStorage && out_stype == kCSRStorage) {
+        BinaryBroadcastCsrDnsCsrImpl<xpu, OP>(ctx, lhs, rhs, req[0], out);
+      } else {
+        LogUnimplementedOp(attrs, ctx, inputs, req, outputs);


Suggest to move LogUnimplementedOp to line 317. Otherwise it may not be called for some unimplemented case

…ors.

eric-haibin-lin

Nice work!

haojin2 · 2018-04-08T07:24:29Z

Update on 4/8:
A small benchmark for this operator is added to compare the new implementation with the fallback implementation

import mxnet as mx
import scipy
import numpy as np
import time

def measure_cost(repeat, f, *args, **kwargs):
    # start bench
    start = time.time()
    results = []
    for i in range(repeat):
        results.append(f(*args, **kwargs))
    for result in results:
        result.wait_to_read()
    end = time.time()
    diff = end - start
    return diff / repeat

def main():
    shape_lhs = (256, 1000000)
    vec = np.random.uniform(size=(256, 1))
    mx_vec = mx.nd.array(vec)
    for density in [0.01, 0.005, 0.001]:
        csr = scipy.sparse.random(256, 1000000, density=density, format = 'csr', dtype=np.float32)
        mx_csr = mx.nd.sparse.csr_matrix((csr.data, csr.indices, csr.indptr), shape=shape_lhs, ctx=mx.cpu())
        mx_dns = mx_csr.tostype('default')
        sparse_cost = 0.0
        dns_cost = 0.0
        for i in range(10):
            sparse_cost += measure_cost(100, mx.nd.broadcast_mul, mx_csr, mx_vec)
            dns_cost += measure_cost(100, mx.nd.broadcast_mul, mx_dns, mx_vec)
        print("%.2f %%" % (density*100), dns_cost / sparse_cost)


if __name__ == "__main__":
    main()

Results on p2.8xlarge instance with commit(2ae0bd5):
(density speedup)
(1.00% 9.453656599452351)
(0.50% 18.406541290116778)
(0.10% 53.18159853487238)

…he#10208) * support broadcast_mul/div(csr, 1Ddense) = csr * address code reviews and support broadcast_mul/div(csr, 2Ddense) = csr * add test for both 1D and 2D dense case * address code review and fix test error * address code review and fix test error * added proper overrides for basic arithmetic functions for sparse tensors. * fix broadcast dimension * address code reviews

haojin2 requested a review from cjolivier01 as a code owner March 22, 2018 18:43

haojin2 force-pushed the broadcast_muldiv branch from b37e1ee to 7aa9bfd Compare March 23, 2018 15:09

eric-haibin-lin suggested changes Mar 23, 2018

View reviewed changes

haojin2 force-pushed the broadcast_muldiv branch from 7b1d7ab to 35d9ae7 Compare March 23, 2018 22:42

haojin2 requested a review from szha as a code owner March 23, 2018 22:42

haojin2 force-pushed the broadcast_muldiv branch 3 times, most recently from 97d198d to f2ee977 Compare March 24, 2018 00:06

haojin2 force-pushed the broadcast_muldiv branch from f2ee977 to ad17b4c Compare March 26, 2018 19:58

haojin2 force-pushed the broadcast_muldiv branch from ad17b4c to 0111aae Compare March 28, 2018 04:11

eric-haibin-lin self-assigned this Mar 28, 2018

eric-haibin-lin reviewed Mar 28, 2018

View reviewed changes

haojin2 requested a review from sergeykolychev as a code owner March 29, 2018 22:40

eric-haibin-lin reviewed Mar 29, 2018

View reviewed changes

haojin2 force-pushed the broadcast_muldiv branch 2 times, most recently from aebfa17 to 2ce2bbb Compare April 2, 2018 17:26

eric-haibin-lin reviewed Apr 2, 2018

View reviewed changes

haojin2 force-pushed the broadcast_muldiv branch from 2ce2bbb to 0556ff3 Compare April 2, 2018 21:27

haojin2 requested a review from thirdwing as a code owner April 2, 2018 21:27

haojin2 force-pushed the broadcast_muldiv branch 2 times, most recently from 68d2cd4 to 6f9d8c2 Compare April 3, 2018 05:28

haojin2 force-pushed the broadcast_muldiv branch from 6f9d8c2 to 09a60a1 Compare April 3, 2018 05:28

eric-haibin-lin reviewed Apr 4, 2018

View reviewed changes

haojin2 force-pushed the broadcast_muldiv branch 4 times, most recently from 3206737 to 1d3d43d Compare April 4, 2018 18:34

eric-haibin-lin reviewed Apr 4, 2018

View reviewed changes

haojin2 force-pushed the broadcast_muldiv branch from 1d3d43d to b56f975 Compare April 4, 2018 23:07

eric-haibin-lin reviewed Apr 4, 2018

View reviewed changes

Hao Jin and others added 8 commits April 5, 2018 17:56

support broadcast_mul/div(csr, 1Ddense) = csr

20ac575

address code reviews and support broadcast_mul/div(csr, 2Ddense) = csr

ae2c4b7

add test for both 1D and 2D dense case

2507d67

address code review and fix test error

fcf9e0b

added proper overrides for basic arithmetic functions for sparse tens…

a36e02c

…ors.

address code review and fix test error

20ef4c4

fix broadcast dimension

2c71eaa

address code reviews

99585fb

haojin2 force-pushed the broadcast_muldiv branch from b56f975 to 99585fb Compare April 5, 2018 17:56

eric-haibin-lin approved these changes Apr 5, 2018

View reviewed changes

eric-haibin-lin merged commit 73a0eb0 into apache:master Apr 5, 2018

haojin2 deleted the broadcast_muldiv branch April 5, 2018 21:48

eric-haibin-lin mentioned this pull request Apr 14, 2018

Add NEWS and README #10545

Merged

haojin2 mentioned this pull request Jul 17, 2018

[Feature Request] broadcast_mul(csr, dense) #9980

Closed

haojin2 added the Sparse label Aug 12, 2019

		# pylint: enable= no-member, protected-access


		def multiply(lhs, rhs):

[MXNET-117] Sparse operator broadcast_mul/div(csr, dense) = csr #10208

[MXNET-117] Sparse operator broadcast_mul/div(csr, dense) = csr #10208

Uh oh!

Conversation

haojin2 commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Essentials

Changes

Comments

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-haibin-lin commented Mar 23, 2018

Uh oh!

marcoabreu commented Mar 26, 2018

Uh oh!

haojin2 commented Mar 26, 2018

Uh oh!

sergeykolychev commented Mar 27, 2018

Uh oh!

haojin2 commented Mar 27, 2018

Uh oh!

sergeykolychev commented Mar 27, 2018

Uh oh!

haojin2 commented Mar 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergeykolychev commented Mar 29, 2018

Uh oh!

haojin2 commented Mar 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergeykolychev Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haojin2 commented Mar 22, 2018 •

edited

Loading

sergeykolychev Mar 30, 2018 •

edited

Loading