Zeroes CSR still needs a valid row_pointer array. #7935

cjolivier01 · 2017-09-18T16:50:21Z

Fix for: #7920

Looks like problem was introduced here
0b13631

In function
void FillZerosCsrImpl(mshadow::Stream *s, NDArray *dst)

Why?
CSR matrix, even if all "zeros", must have m + 1 items in its row pointer (csr::IndPtr) array. Current implementation leaves all aux arrays empty.

While I was here, added a couple more assertions and also reduced memory allocation for situations where lhs and rhs are the same array.

Added unit test.

eric-haibin-lin

The intention of such _zeros implementation was to delay memory allocation until a sparse tensor has some non-zeros. If we want to enforce that a CSR ndarray always has valid indptr array, I believe this(_zeros) is not the only place that violates such constraint. Because a CSR NDArray is constructed with default shape for indptr = (0, )
https://github.com/apache/incubator-mxnet/blob/master/include/mxnet/ndarray.h#L140`

Do you think it's possible for operators to work with indptr being empty?
@reminisce

eric-haibin-lin · 2017-09-18T21:38:46Z

src/operator/tensor/init_op.h

-// Fill a CSR NDArray with zeros by updating the aux shape.
+/*! \brief Fill a CSR NDArray with zeros by updating the aux shape
+ *
+ * @tparam xpu - cpu or gpu


tparam -> param

what? cryptic comment :)

tparam = template parameter. It's actually named by the IDE.

https://www.stack.nl/~dimitri/doxygen/manual/commands.html

Sorry about that. Learnt a new thing today 👍

reminisce · 2017-09-18T22:16:03Z

I think it's okay for operators to work on a zero csr with empty indptr as long as the csr is checked if zero before performing any regular operations like the one exposed in #7920. In addition to what @eric-haibin-lin said about the intention of delaying memory allocation, I think it's also the intended way to avoid unnecessary memory allocation for zero csr tensors. We can provide a util function checking whether a sparse tensor (csr or rsp) is zero for being used in operator implementation.

cjolivier01 · 2017-09-19T01:10:31Z

I respectfully disagree. Maybe something like empty() should return uninitialized, but returning an Invald CSR array from zeros() and expecting the consuming function to initialize the array seems backwards and difficult to maintain. zeros() should return a CSR that appears as a matrix of zeroes. It is not doing that.

cjolivier01 · 2017-09-19T01:12:20Z

Also note that CheckAndAlloc() isn't going to fill that allocated array with zeros. And indptr array size isn't going to change so it doesn't need to be reallocated larr when the array is used.

cjolivier01 · 2017-09-19T01:23:27Z

While an unintialized RSP matrix is a valid all-zero matrix, this is not the case for CSR. Thus RSP returns a valid matrix and CSR does not.

reminisce · 2017-09-19T04:11:09Z

I agree that an all zero initialized indptr in a zero csr sounds more correct from the definition point of view of csr and alleviates the burden on writing client code. If memory allocation is not a concern, I'm okay with the change.

cjolivier01 · 2017-09-19T05:38:38Z

I don't see any burden on memory allocation greater than normal NDArray.zeros() allocation. It'll be less, actually, more often than not. But it's certainly not some "abnormal" memory allocation situation by any stretch of the imagination.

…

On Mon, Sep 18, 2017 at 9:11 PM reminisce ***@***.***> wrote: I agree that an all zero initialized indptr in a zero csr sounds more correct from the definition point of view of csr and alleviates the burden on writing client code. If memory allocation is not a concern, I'm okay with the change. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7935 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKts_fec7ZLPQAiqDc4iAv0bm2YGYiY-ks5sjz7xgaJpZM4PbNPO> .

…piled file, the gpu version is compiled via cuda directly in init_op.cu

eric-haibin-lin · 2017-09-19T17:13:53Z

I agree that we should return a CSR array with valid indptr array for that zeros() function. Should we also update the default shape for indptr to shape(num_rows + 1) in ndarray.h?

cjolivier01 · 2017-09-26T15:20:16Z

@eric-haibin-lin As far as I can tell, it already is (num_rows + 1):
dst->CheckAndAllocAuxData(csr::kIndPtr, mshadow::Shape1(dst->shape()[0] + 1));

…because so many errors coming from this test.

piiswrong · 2017-09-27T18:57:02Z

tests/python/unittest/test_kvstore.py

@@ -241,64 +241,64 @@ def test_get_type():
    kvtype = 'local_allreduce_cpu'
    kv = mx.kv.create(kvtype)
    assert kv.type == kvtype
-
-def test_invalid_pull():


same as before, looks like Haibin fixed maybe

eric-haibin-lin · 2017-10-01T22:05:48Z

tests/python/unittest/test_sparse_operator.py

+def test_elemwise_csr_same_zeros():
+    # Zeroes
+    a = mx.nd.sparse.zeros('csr', (1,1))
+    b = mx.nd.elemwise_add(a,a)


binary ops for csr have been disabled in #7947 (currently inferring dense output).
https://github.com/apache/incubator-mxnet/blob/master/src/operator/tensor/elemwise_binary_op.h#L573
Could you add it back with FInferStorage = ElemwiseStorageType<2, 1, true, true, true>) and all the necessary unit tests for CSR binary ops?

done a different way

eric-haibin-lin · 2017-10-01T22:08:20Z

src/operator/tensor/elemwise_binary_op-inl.h

+
+  // Input and output should have the same number of row pointer items (m + 1)
+  CHECK_EQ(output.aux_shape(csr::kIndPtr), lhs.aux_shape(csr::kIndPtr));
+  CHECK_EQ(output.aux_shape(csr::kIndPtr), rhs.aux_shape(csr::kIndPtr));


We should remove SparseTempStorage class. @piiswrong suggest we register FResourceRequest for all sparse operators that requires temp space.

I would have expected you changed that in your PR... Your PR handled no CSR types?

I only refactored fallback logic in the previous PR.. And we don't have csr binary op tests back then..

This is incredibly messy, IMHO

Adding identical FResourceRequest items for everything.

…e_fix

piiswrong · 2017-10-09T22:12:19Z

src/operator/tensor/sparse_retain-inl.h

@@ -267,7 +267,7 @@ void SparseRetainOpForwardRspImpl(mshadow::Stream<xpu> *s,
  if (!input_nd.storage_initialized()
      || idx_data.Size() == 0U
      || input_nd.shape()[0] == 0) {
-    FillZerosRspImpl(s, *output_nd);


This was changed on purpose. Why revert this?

It was a conflict and causing a compile error because my PR changes this function. Why is it changed to const reference?

When resolving the conflict I used the surrounding functions as a reference:

inline void FillDnsZerosRspImpl(mshadow::Stream *s, NDArray *dst)
void PopulateFullIdxRspImpl(mshadow::Stream *s, NDArray *dst)

btw why is there so much const reference being passed around when the object is then modified? I see this everywhere. Is this because lint complains anout not using a pointer for modified items rather than a non-const reference?

NDArray is a pointer type. So its ok to modify the content of the object it points to even if the pointer is const

I think you mean NDArray::ptr_ is a const pointer type? NDArray was a const reference.

ok, reverted

piiswrong · 2017-10-09T22:13:33Z

src/operator/tensor/init_op.h

 }

-// Fill a CSR NDArray with zeros by updating the aux shape.
-template<typename xpu>
-void FillZerosCsrImpl(mshadow::Stream<xpu> *s, const NDArray& dst) {


piiswrong · 2017-10-10T08:01:19Z

src/ndarray/ndarray.cc

@@ -402,7 +402,7 @@ inline void CopyFromToCsrImpl(const NDArray& from, const NDArray& to, RunContext
  // if source storage is not initialized, fill destination with zeros
  auto s = ctx.get_stream<to_xpu>();
  if (!from.storage_initialized()) {
-    op::FillZerosCsrImpl<to_xpu>(s, to);
+    op::FillZerosCsrImpl(s, const_cast<NDArray *>(&to));


const cast should be avoided

* Fix for: apache#7920 * lint * remove unused variable warning * Since GPU version of FillZerosCsrImpl() is called from a non-cuda-compiled file, the gpu version is compiled via cuda directly in init_op.cu * Trigger build * This test case is BS. I can't even tell what's wrong on the CI build because so many errors coming from this test. * Update test_kvstore.py * Update CMakeLists.txt * merge fix * Trigger build * Fix 'dest' item in FIllXXX calls * Revert NDARray as pointer in Fillxxx

Fix for: apache#7920

016f224

cjolivier01 requested review from mli and piiswrong as code owners September 18, 2017 16:50

cjolivier01 mentioned this pull request Sep 18, 2017

csr binary operator bug #7920

Closed

lint

89906cb

eric-haibin-lin reviewed Sep 18, 2017

View reviewed changes

Olivier added 3 commits September 19, 2017 08:35

Merge remote-tracking branch 'apache/master' into sparse_fix

3418492

remove unused variable warning

6c2de73

Since GPU version of FillZerosCsrImpl() is called from a non-cuda-com…

1449f67

…piled file, the gpu version is compiled via cuda directly in init_op.cu

Olivier added 3 commits September 20, 2017 11:54

Merge remote-tracking branch 'apache/master' into sparse_fix

34795b1

Merge remote-tracking branch 'apache/master' into sparse_fix

743508d

Trigger build

5eb2965

Olivier added 3 commits September 26, 2017 09:19

Merge branch 'master' into sparse_fix

6e347e3

This test case is BS. I can't even tell what's wrong on the CI build …

37ed37b

…because so many errors coming from this test.

Merge remote-tracking branch 'apache/master' into sparse_fix

ab866b1

piiswrong reviewed Sep 27, 2017

View reviewed changes

Olivier and others added 3 commits September 27, 2017 15:19

Merge remote-tracking branch 'apache/master' into sparse_fix

b677b25

Update test_kvstore.py

02bdf27

Update CMakeLists.txt

a7718d0

eric-haibin-lin reviewed Oct 2, 2017

View reviewed changes

Olivier added 2 commits October 4, 2017 17:01

Merge remote-tracking branch 'apache/master' into sparse_fix

7e66cb6

Merge branch 'sparse_fix' of github.com:/cjolivier01/mxnet into spars…

17822bc

…e_fix

Olivier added 4 commits October 9, 2017 08:57

Merge remote-tracking branch 'apache/master' into sparse_fix

114381c

merge fix

c141661

Trigger build

9dccee7

Fix 'dest' item in FIllXXX calls

2b9355d

piiswrong reviewed Oct 9, 2017

View reviewed changes

piiswrong reviewed Oct 10, 2017

View reviewed changes

Olivier added 2 commits October 10, 2017 08:52

Revert NDARray as pointer in Fillxxx

83516b8

Merge remote-tracking branch 'apache/master' into sparse_fix

e14f3eb

piiswrong merged commit a48991f into apache:master Oct 10, 2017

cjolivier01 deleted the sparse_fix branch October 10, 2017 18:59

eric-haibin-lin mentioned this pull request Nov 7, 2017

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

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zeroes CSR still needs a valid row_pointer array. #7935

Zeroes CSR still needs a valid row_pointer array. #7935

cjolivier01 commented Sep 18, 2017

eric-haibin-lin left a comment

eric-haibin-lin Sep 18, 2017

cjolivier01 Sep 18, 2017

cjolivier01 Sep 18, 2017

cjolivier01 Sep 18, 2017

eric-haibin-lin Sep 18, 2017

reminisce commented Sep 18, 2017

cjolivier01 commented Sep 19, 2017

cjolivier01 commented Sep 19, 2017

cjolivier01 commented Sep 19, 2017

reminisce commented Sep 19, 2017

cjolivier01 commented Sep 19, 2017 via email

eric-haibin-lin commented Sep 19, 2017

cjolivier01 commented Sep 26, 2017 •

edited

piiswrong Sep 27, 2017

cjolivier01 Sep 27, 2017

eric-haibin-lin Oct 1, 2017

cjolivier01 Oct 4, 2017

eric-haibin-lin Oct 1, 2017

cjolivier01 Oct 4, 2017

eric-haibin-lin Oct 4, 2017

cjolivier01 Oct 5, 2017

cjolivier01 Oct 5, 2017

eric-haibin-lin Oct 5, 2017

cjolivier01 Oct 9, 2017

piiswrong Oct 9, 2017

cjolivier01 Oct 9, 2017

piiswrong Oct 10, 2017 •

edited

cjolivier01 Oct 10, 2017 •

edited

cjolivier01 Oct 10, 2017

piiswrong Oct 9, 2017

cjolivier01 Oct 9, 2017

piiswrong Oct 10, 2017

Zeroes CSR still needs a valid row_pointer array. #7935

Zeroes CSR still needs a valid row_pointer array. #7935

Conversation

cjolivier01 commented Sep 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reminisce commented Sep 18, 2017

cjolivier01 commented Sep 19, 2017

cjolivier01 commented Sep 19, 2017

cjolivier01 commented Sep 19, 2017

reminisce commented Sep 19, 2017

cjolivier01 commented Sep 19, 2017 via email

eric-haibin-lin commented Sep 19, 2017

cjolivier01 commented Sep 26, 2017 • edited

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

piiswrong Oct 10, 2017 • edited

Choose a reason for hiding this comment

cjolivier01 Oct 10, 2017 • edited

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

cjolivier01 commented Sep 26, 2017 •

edited

piiswrong Oct 10, 2017 •

edited

cjolivier01 Oct 10, 2017 •

edited