[sparse] slice for csr on two dimensions, cpu implementation #8331

ZiyueHuang · 2017-10-18T04:00:50Z

Description

slice_axis for csr, cpu implementation. This is used in cases like Wide & Deep model, e.g., slice the linear features to feed into wide model.

As a feature request in #8168.

cc @eric-haibin-lin for review

Checklist

Essentials

Passed code style checking (make lint)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage
For user-facing API changes, API doc string has been updated.
To my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

slice_axis for csr, cpu implementation, add unittest

Comments

If this change is a backward incompatible change, why must this change be made.
Intersting edge cases to note here

eric-haibin-lin · 2017-10-22T17:03:17Z

src/operator/tensor/matrix_op-inl.h

+  CHECK_EQ(in_attrs->size(), 1);
+  CHECK_EQ(out_attrs->size(), 1);
+  const SliceAxisParam& param = nnvm::get<SliceAxisParam>(attrs.parsed);
+  const auto& in_stype = in_attrs->at(0);


No need for & if in_stype is not changed.

eric-haibin-lin · 2017-10-22T17:09:29Z

src/operator/tensor/matrix_op-inl.h

+
+template<typename xpu>
+void SliceAxisEx(const nnvm::NodeAttrs& attrs,
+          const OpContext& ctx,


nit: indentation

eric-haibin-lin · 2017-10-22T17:10:05Z

src/operator/tensor/matrix_op-inl.h

+
+  const SliceAxisParam& param = nnvm::get<SliceAxisParam>(attrs.parsed);
+  auto in_stype = inputs[0].storage_type();
+  CHECK_NE(in_stype, kDefaultStorage)


I think you can remove this check and print operator_info(ctx, ..) in line 1060

eric-haibin-lin · 2017-10-22T17:11:57Z

src/operator/tensor/matrix_op-inl.h

+    } else if (param.axis == 1) {
+      SliceAxisOneCsrImpl<xpu>(param, ctx, inputs[0], req[0], outputs[0]);
+    } else {
+      LOG(FATAL) << "CSRNDArray is only for 2-D shape";


Does it fail with negative axis? I think GetSliceAxisParams already handles negative axis for you

eric-haibin-lin · 2017-10-22T17:12:37Z

tests/python/unittest/test_sparse_ndarray.py

@@ -127,9 +127,27 @@ def check_slice_nd_csr_fallback(shape):
        result_dense = mx.nd.slice(mx.nd.array(A2), begin=(start, shape[1] - 1), end=(end + 1, shape[1]))
        assert same(result_dense.asnumpy(), result.asnumpy())

-    shape = (rnd.randint(2, 10), rnd.randint(1, 10))
+    def check_sparse_nd_csr_slice_axis(shape):


let's also add some test cases for negative axis

eric-haibin-lin · 2017-10-22T17:41:45Z

src/operator/tensor/matrix_op-inl.h

+  CHECK_NE(req, kWriteInplace) << "kWriteInplace for SliceAxis on CSR input is not supported";
+  int axis, begin, end;
+  GetSliceAxisParams(param, in.shape(), &axis, &begin, &end);
+  int indptr_len = in.shape()[0] + 1;


Use nnvm::dim_t (int64_t) instead because shape[i] is 64 bits

eric-haibin-lin · 2017-10-22T17:41:58Z

src/operator/tensor/matrix_op-inl.h

+
+template<typename xpu>
+void SliceAxisOneCsrImpl(const SliceAxisParam &param, const OpContext& ctx,
+                  const NDArray &in, OpReqType req, const NDArray &out) {


nit: indentation

eric-haibin-lin · 2017-10-22T17:44:04Z

src/operator/tensor/matrix_op-inl.h

+        RType *out_indptr = out.aux_data(kIndPtr).dptr<RType>();
+        int nnz = 0;
+        out_indptr[0] = 0;
+        for (int i=0; i < indptr_len - 1; i++) {


also use nnvm::dim_t for i and j

eric-haibin-lin · 2017-10-22T17:47:02Z

src/operator/tensor/matrix_op-inl.h

+        for (int i=0; i < indptr_len - 1; i++) {
+          out_indptr[i+1] = out_indptr[i];
+          for (int j=in_indptr[i]; j < in_indptr[i+1]; j++) {
+            if (in_idx[j] >= begin && in_idx[j] < end) {


continue if in_idx[j] >= end instead of scanning the rest, since indices are sorted per row?

eric-haibin-lin · 2017-10-22T17:49:21Z

src/operator/tensor/matrix_op-inl.h

+        DType *out_data = out.data().dptr<DType>();
+
+        Stream<xpu> *s = ctx.get_stream<xpu>();
+        Kernel<SliceAxisOneCsrAssign, xpu>::Launch(s, indptr_len-1, out_idx, out_data,


Does it work when nnz = 0? Is that tested?

Yes. If nnz=0, kernel launch will return immediately. Test for slice_axis(zeros, ...) is added.

piiswrong · 2017-10-29T03:44:28Z

I think slice axis is deprecated, we are using slice now

ZiyueHuang · 2017-10-29T04:06:39Z

OK, I will change it to slice.

eric-haibin-lin

A few comments regarding docs. Also adding @anirudh2290 for review

eric-haibin-lin · 2017-10-30T03:32:45Z

src/operator/tensor/matrix_op.cc

@@ -264,7 +264,7 @@ The resulting array's *k*-th dimension contains elements
 from the *k*-th dimension of the input array with the open range ``[b_k, e_k)``.

 For an input array of non-default storage type(e.g. `csr` or `row_sparse`), it only supports


I think row_sparse is not supported for slice. Let's remove this sentence in the doc.

The storage type of ``slice`` output depends on storage types of inputs: - slice(csr) = csr - slice(default) = default

eric-haibin-lin · 2017-10-30T22:45:09Z

src/operator/tensor/matrix_op-inl.h

@@ -601,18 +590,127 @@ void SliceCsrImpl(const SliceParam &param, const OpContext& ctx,
  });
 }

+// slice a CSRNDArray for two dimensions


Let's add more documentation for the kernels like this one https://github.com/apache/incubator-mxnet/blob/master/src/operator/tensor/dot-inl.cuh#L40

anirudh2290

Thank you for adding this operator!

anirudh2290 · 2017-10-31T07:26:55Z

src/operator/tensor/matrix_op-inl.h

  out.CheckAndAllocAuxData(kIndPtr, Shape1(indptr_len));
-  if (!in.storage_initialized()) {


What happens here if input is a CSR Array with all zeroes ?

Thanks for your comments. If input is zeros, kernel launch will return immediately. Unittest for zeros input case is added.

Is that still true on GPU, when we add GPU support? This PR is dealing with some bugs for zero inputs for dot operator #8470

For CSRNDArray, storage_initialized() return aux_shape(0).Size() != 0, I think it is always true for a valid CSRNDArray except for rank-0 array.

Changed to returning csr zeros immediately if nnz=0.

anirudh2290 · 2017-10-31T07:27:00Z

src/operator/tensor/matrix_op-inl.h

-  if (!in.storage_initialized()) {
-    out.set_aux_shape(kIndPtr, Shape1(0));
-    return;
-  }
  // assume idx indptr share the same type
  MSHADOW_IDX_TYPE_SWITCH(in.aux_type(kIndPtr), RType, {
    MSHADOW_IDX_TYPE_SWITCH(in.aux_type(kIdx), IType, {
      MSHADOW_TYPE_SWITCH(in.dtype(), DType, {
        auto in_indptr = in.aux_data(kIndPtr).dptr<RType>();
        auto out_indptr = out.aux_data(kIndPtr).dptr<RType>();


Can we avoid auto for in_indptr and out_indptr

anirudh2290 · 2017-10-31T07:29:03Z

src/operator/tensor/matrix_op-inl.h

@@ -592,7 +581,7 @@ void SliceCsrImpl(const SliceParam &param, const OpContext& ctx,
        auto out_idx = out.aux_data(kIdx).dptr<IType>();
        auto in_data = in.data().dptr<DType>();


Can we avoid auto here and use IType and DType

anirudh2290 · 2017-10-31T07:48:24Z

src/operator/tensor/matrix_op-inl.h

+          out_indptr[i+1] = out_indptr[i];
+          for (RType j = in_indptr[i + begin_row];
+               j < in_indptr[i + begin_row + 1]; j++) {
+            if (in_idx[j] >= end_col) {


Can we add one line comment for the if, else if logic here. Also why not just if (in_idx[j] >= begin_col && in_idx < end_col) ?

anirudh2290 · 2017-10-31T08:04:29Z

src/operator/tensor/matrix_op-inl.h

+                                  const int begin, const int end) {
+    RType ind = out_indptr[i];
+    for (RType j = in_indptr[i]; j < in_indptr[i+1]; j++) {
+      if (in_idx[j] >= end) {


Any reason to not just do if (in_idx[j] >= begin_col && in_idx < end_col)

Indices of csr ndarray is in ascending order per row. So if indice >= end, there is no need to continue the loop.

I was suggesting this change for readability. Also, you would be doing the checks for all in_idx[j] < begin_col which will be avoided with the change.

I used this condition, in_idx[j] >= begin_col && in_idx < end_col, at the first time. But according to @eric-haibin-lin 's comments, this logic should be changed to a if/else logic which can jump out of the loop since indices are sorted per row.

…8331) * slice axis for csr (cpu impl) * fix indice bug and use kernel launch * small fix * misc updates to address comments * fix type * csr slice * unittest * fix lint * address comments * return csr zeros before kernel launch if nnz=0 * fix

ZiyueHuang added 4 commits October 18, 2017 11:49

slice axis for csr (cpu impl)

3733381

fix indice bug and use kernel launch

b680fac

Merge remote-tracking branch 'upstream/master' into slice-axis-csr

b1ba370

small fix

ec07bf4

eric-haibin-lin reviewed Oct 22, 2017

View reviewed changes

ZiyueHuang added 3 commits October 23, 2017 22:27

misc updates to address comments

688c212

fix type

1fb751c

Merge remote-tracking branch 'upstream/master' into slice-axis-csr

72b6d65

ZiyueHuang added 4 commits October 29, 2017 12:47

Merge remote-tracking branch 'upstream/master' into slice-axis-csr

50b4eb4

csr slice

2c3849b

unittest

086ba4a

fix lint

d7c99ec

ZiyueHuang changed the title ~~[sparse] slice_axis for csr, cpu implementation~~ [sparse] slice for csr on two dimensions, cpu implementation Oct 29, 2017

eric-haibin-lin reviewed Oct 30, 2017

View reviewed changes

anirudh2290 reviewed Oct 31, 2017

View reviewed changes

ZiyueHuang added 5 commits November 1, 2017 00:07

address comments

9b16b69

Merge remote-tracking branch 'upstream/master' into slice-axis-csr

5f50690

return csr zeros before kernel launch if nnz=0

3e1445c

fix

0988a3b

Merge remote-tracking branch 'upstream/master' into slice-axis-csr

a63e6e3

eric-haibin-lin self-assigned this Nov 8, 2017

eric-haibin-lin approved these changes Nov 8, 2017

View reviewed changes

piiswrong merged commit bf2336c into apache:master Nov 8, 2017

ZiyueHuang deleted the slice-axis-csr branch January 30, 2018 11:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sparse] slice for csr on two dimensions, cpu implementation #8331

[sparse] slice for csr on two dimensions, cpu implementation #8331

ZiyueHuang commented Oct 18, 2017 •

edited

Loading

eric-haibin-lin Oct 22, 2017

eric-haibin-lin Oct 22, 2017

eric-haibin-lin Oct 22, 2017

eric-haibin-lin Oct 22, 2017

eric-haibin-lin Oct 22, 2017

ZiyueHuang Oct 23, 2017

eric-haibin-lin Oct 22, 2017

eric-haibin-lin Oct 22, 2017

eric-haibin-lin Oct 22, 2017

eric-haibin-lin Oct 22, 2017

eric-haibin-lin Oct 22, 2017

ZiyueHuang Oct 23, 2017

piiswrong commented Oct 29, 2017

ZiyueHuang commented Oct 29, 2017

eric-haibin-lin left a comment

eric-haibin-lin Oct 30, 2017

eric-haibin-lin Oct 30, 2017

anirudh2290 left a comment

anirudh2290 Oct 31, 2017

ZiyueHuang Oct 31, 2017

eric-haibin-lin Oct 31, 2017

ZiyueHuang Oct 31, 2017

ZiyueHuang Oct 31, 2017

anirudh2290 Oct 31, 2017

anirudh2290 Oct 31, 2017

anirudh2290 Oct 31, 2017

anirudh2290 Oct 31, 2017

ZiyueHuang Oct 31, 2017 •

edited

Loading

anirudh2290 Oct 31, 2017

ZiyueHuang Nov 1, 2017

		@@ -264,7 +264,7 @@ The resulting array's k-th dimension contains elements
		from the k-th dimension of the input array with the open range ``[b_k, e_k)``.

		For an input array of non-default storage type(e.g. `csr` or `row_sparse`), it only supports

		out.CheckAndAllocAuxData(kIndPtr, Shape1(indptr_len));
		if (!in.storage_initialized()) {

		@@ -592,7 +581,7 @@ void SliceCsrImpl(const SliceParam &param, const OpContext& ctx,
		auto out_idx = out.aux_data(kIdx).dptr<IType>();
		auto in_data = in.data().dptr<DType>();

[sparse] slice for csr on two dimensions, cpu implementation #8331

[sparse] slice for csr on two dimensions, cpu implementation #8331

Conversation

ZiyueHuang commented Oct 18, 2017 • edited Loading

Description

Checklist

Essentials

Changes

Comments

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

piiswrong commented Oct 29, 2017

ZiyueHuang commented Oct 29, 2017

eric-haibin-lin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anirudh2290 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZiyueHuang Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZiyueHuang commented Oct 18, 2017 •

edited

Loading

ZiyueHuang Oct 31, 2017 •

edited

Loading