[MXNET-117] [WIP] [DO NOT MERGE] Sparse operator broadcast_mul/div(csr, dense) = csr by haojin2 · Pull Request #10150 · apache/mxnet

haojin2 · 2018-03-18T17:42:44Z

Description

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

Checklist

Essentials

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

Comments

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)

cjolivier01 · 2018-03-19T05:02:05Z

  }
 };
+
+template<typename DType, typename CType, typename RType, int req, typename OP>


can the template types be at function level and be inferred automatically by the arguments passed, or are you going for type checking?

Agree with applying these as function's template arguments instead of the class's.

Will make that change soon.

cjolivier01 · 2018-03-19T05:05:13Z

+    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, {
+          Kernel<csr_dns_csr_broadcast_kernel<DType, CType, RType, req_type, OP>, xpu>::Launch(


yeah, you can probably get by without passing all of these template parameters

cjolivier01 · 2018-03-19T05:06:10Z

+  bool col_vec = (dns.shape()[0] == csr.shape()[0])? true : false;
+  if (!csr.storage_initialized()) {
+    FillZerosCsrImpl(s, output);
+    return;


would just an else block rather than a return be more readable?

Sure, will do that.

cjolivier01 · 2018-03-19T05:07:38Z

+      //           out_stype == kDefaultStorage) {
+      //  BinaryBroadCastCsrDnsDnsImpl(ctx, inputs[0], input[1], req[0], outputs[0]);
+      } else {
+        LogUnimplementedOp(attrs, ctx, inputs, req, outputs);


would catching this in the storage type inference and then doing a fallback not work for this case?

If we get really big sparse matrices as inputs then fallback may not be a better choice than throwing?

Usually in the other cases, it falls back and will print a warning the first time. Are there other cases where it just throws an error rather than falling back?

I agree that throwing an error is not desirable and blocks users from what they want to do. The problem is that finferstorage is not aware of shape and dtype, and dispatch only based on dev_mask and storage types. And for this sparse broadcast operator it's a lot of work to implement cases for 2-D and 3-D.

Maybe a temporary walk-around is to fallback inside the operator..

reminisce · 2018-03-19T16:34:26Z

 }

+template<typename xpu, typename OP>
+void BinaryBroadCastCsrDnsCsrImpl(const OpContext& ctx,


BroadCast -> Broadcast

reminisce · 2018-03-19T16:51:49Z

  }
 };
+
+template<typename DType, typename CType, typename RType, int req, typename OP>


Agree with applying these as function's template arguments instead of the class's.

eric-haibin-lin · 2018-03-19T18:14:52Z

  return true;
 }

+inline bool BinaryBroadcastStorageTypeCsr(const nnvm::NodeAttrs& attrs,


No need to put Csr in the title since we may add row_sparse in the same function in future. Also the name is confusing because this is only for mul/div

Changed to BinaryBroadcastMulStorageType

eric-haibin-lin · 2018-03-19T18:16:00Z

+                                          std::vector<int>* out_attrs) {
+  CHECK_EQ(in_attrs->size(), 2U);
+  CHECK_EQ(out_attrs->size(), 1U);
+  const int in1_stype = in_attrs->at(0);


I'd think left/right-hand side (lhs/rhs) is a better name compared to in1/in2

eric-haibin-lin · 2018-03-19T18:19:29Z

@@ -122,6 +122,8 @@ Example::



Need to clarify what's supported/not supported in doc like https://mxnet.incubator.apache.org/versions/master/api/python/ndarray/sparse.html#mxnet.ndarray.sparse.dot

Please only add the docs for broadcast_mul here.

eric-haibin-lin · 2018-03-19T18:25:10Z

+      //           out_stype == kDefaultStorage) {
+      //  BinaryBroadCastCsrDnsDnsImpl(ctx, inputs[0], input[1], req[0], outputs[0]);
+      } else {
+        LogUnimplementedOp(attrs, ctx, inputs, req, outputs);


I agree that throwing an error is not desirable and blocks users from what they want to do. The problem is that finferstorage is not aware of shape and dtype, and dispatch only based on dev_mask and storage types. And for this sparse broadcast operator it's a lot of work to implement cases for 2-D and 3-D.

Maybe a temporary walk-around is to fallback inside the operator..

eric-haibin-lin · 2018-03-19T18:26:04Z

+  using namespace mxnet_op;
+  using namespace csr;
+  CHECK_EQ(dns.shape().ndim(), 1) << "input dense should be a vector";
+  mshadow::Stream<xpu> *s = ctx.get_stream<xpu>();


Need to check req != kAddTo / kWriteInplace

eric-haibin-lin · 2018-03-21T20:51:20Z

+  MSHADOW_XINLINE static void Map(int row, const DType *csr_data, const CType *csr_indices,
+                                  const RType *csr_indptr, const DType *dns,
+                                  DType *out, const nnvm::dim_t row_length, bool col_vec) {
+    nnvm::dim_t curr_row_i = csr_indptr[row];


eric-haibin-lin · 2018-03-21T20:52:42Z

+  template <typename DType, typename CType, typename RType>
+  MSHADOW_XINLINE static void Map(int row, const DType *csr_data, const CType *csr_indices,
+                                  const RType *csr_indptr, const DType *dns,
+                                  DType *out, const nnvm::dim_t row_length, bool col_vec) {


col_vec could be part of the template

This flag may actually be abandoned later, will do if it still exists later.

eric-haibin-lin · 2018-03-21T20:53:01Z

@@ -122,6 +122,8 @@ Example::



Please only add the docs for broadcast_mul here.

eric-haibin-lin · 2018-03-21T20:53:12Z

@@ -156,6 +161,8 @@ Example::



Please update doc for broadcast_div

haojin2 requested a review from cjolivier01 as a code owner March 18, 2018 17:42

haojin2 force-pushed the broadcast_1D_mul branch from f0f77bf to 20ba3d0 Compare March 18, 2018 17:55

cjolivier01 reviewed Mar 19, 2018

View reviewed changes

reminisce reviewed Mar 19, 2018

View reviewed changes

eric-haibin-lin reviewed Mar 19, 2018

View reviewed changes

haojin2 force-pushed the broadcast_1D_mul branch from 20ba3d0 to c4f21dc Compare March 20, 2018 21:19

haojin2 changed the title ~~[WIP] [DO NOT MERGE] Sparse operator broadcast_mul/div(csr, dense) = csr~~ [MXNET-117] [WIP] [DO NOT MERGE] Sparse operator broadcast_mul/div(csr, dense) = csr Mar 20, 2018

haojin2 force-pushed the broadcast_1D_mul branch 2 times, most recently from ddfa5b8 to 4626999 Compare March 21, 2018 00:06

eric-haibin-lin reviewed Mar 21, 2018

View reviewed changes

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

3002bf7

haojin2 force-pushed the broadcast_1D_mul branch 2 times, most recently from d086e34 to edc6e28 Compare March 22, 2018 16:44

address code reviews

e748187

haojin2 force-pushed the broadcast_1D_mul branch from edc6e28 to e748187 Compare March 22, 2018 17:08

haojin2 closed this Mar 22, 2018

haojin2 mentioned this pull request Mar 22, 2018

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

Merged

9 tasks

Conversation

haojin2 commented Mar 18, 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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

haojin2 commented Mar 18, 2018 •

edited

Loading