eye for dense and sparse #8225

ZiyueHuang · 2017-10-11T16:24:39Z

Description

eye for storage_type of default, csr, row_sparse(fallback to default).
As a feature requested 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

eye for default storage type, add unitest
eye for csr storage type, add unitest
eye for row_sparse storage type, add unitest

Comments

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

Fix continuous build.

eric-haibin-lin · 2017-10-12T16:55:35Z

src/operator/tensor/init_op.h

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


please fix indentation

eric-haibin-lin · 2017-10-12T17:07:13Z

src/operator/tensor/init_op.h

+}
+
+template<int req>
+struct eye_fill_impl {


nit: maybe eye_dns_fill is a better name?

eric-haibin-lin · 2017-10-12T17:12:34Z

python/mxnet/ndarray/ndarray.py

+    --------
+    >>> mx.nd.eye(1, 2)
+
+    [[ 1.  0.]]


let put a simple example and a complex one with k!= 0

eric-haibin-lin · 2017-10-12T17:13:30Z

python/mxnet/ndarray/ndarray.py

+        An optional list of types of the aux data for RowSparseNDArray or CSRNDArray
+        (default values depends on the storage type)
+
+    Returns


Returns ------- NDArray A created array

eric-haibin-lin · 2017-10-12T17:14:35Z

python/mxnet/ndarray/ndarray.py

+    if ctx is None:
+        ctx = Context.default_ctx
+    dtype = mx_real_t if dtype is None else dtype
+    # pylint: disable= no-member, protected-access


are these pylint disable necessary?

I just follow the way in zeros. https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/ndarray/ndarray.py#L2928

eric-haibin-lin · 2017-10-12T17:15:31Z

python/mxnet/ndarray/utils.py

+        (default values depends on the storage type)
+
+    Returns
+    -------


lets put csrndarray as the return type here

Done.

NDArray or CSRNDArray A created array

eric-haibin-lin · 2017-10-12T17:16:56Z

src/operator/tensor/init_op.h

+    .add_enum("float64", mshadow::kFloat64)
+    .add_enum("float16", mshadow::kFloat16)
+    .add_enum("uint8", mshadow::kUint8)
+    .add_enum("int32", mshadow::kInt32)


can we support int64, too?

eric-haibin-lin · 2017-10-12T17:17:09Z

src/operator/tensor/init_op.h

+
+template<typename ParamType>
+inline bool InitEyeShape(const nnvm::NodeAttrs& attrs,
+                      std::vector<TShape> *in_attrs,


pls fix indentation

eric-haibin-lin · 2017-10-12T17:18:20Z

src/operator/tensor/init_op.h

+  CHECK_EQ(req[0], kWriteTo) << "kWriteTo is expected for EyeFillEx";
+  if (stype == kCSRStorage) {
+    NDArray nd(outputs[0]);
+    EyeFillCsr<xpu>(s, nd, param.N, param.M, param.k);


you can directly pass outputs[0] to EyeFillCsr

eric-haibin-lin · 2017-10-12T17:20:11Z

src/operator/tensor/init_op.h

+void EyeFillCsr(mshadow::Stream<xpu> *s, const NDArray& out,
+                const nnvm::dim_t N, const nnvm::dim_t M, const nnvm::dim_t k) {
+  const nnvm::dim_t num_cols = M > 0 ? M : N;
+  const nnvm::dim_t nnz = k > 0 ? std::min(std::max(num_cols - std::abs(k), (nnvm::dim_t)0), N) :


I think this line is too long..

eric-haibin-lin

Looks good. Just some final comments..

eric-haibin-lin · 2017-10-14T18:49:42Z

python/mxnet/ndarray/sparse.py

+        An optional device context (default is the current default context)
+    dtype : str or numpy.dtype, optional
+        An optional value type (default is `float32`)
+    aux_types: list of numpy.dtype, optional


aux_types will be removed by #8269
let's also not expose aux_types to users here, either. just using the default one is fine

eric-haibin-lin · 2017-10-14T18:50:20Z

python/mxnet/ndarray/utils.py

+        An optional device context (default is the current default context)
+    dtype: str or numpy.dtype, optional
+        An optional value type (default is `float32`)
+    aux_types: list of numpy.dtype, optional


same here for aux-types

eric-haibin-lin · 2017-10-14T18:56:31Z

tests/python/unittest/test_ndarray.py

@@ -730,6 +730,13 @@ def test_output():
    assert_almost_equal(out.asnumpy(), zeros.asnumpy())
    mx.nd.full(shape, 2, out=out)
    assert_almost_equal(out.asnumpy(), ones.asnumpy() * 2)
+    import random


nit: any reason why np.random.randint is not used here?

eric-haibin-lin · 2017-10-14T18:57:03Z

src/operator/tensor/init_op.h

+    .add_enum("float16", mshadow::kFloat16)
+    .add_enum("uint8", mshadow::kUint8)
+    .add_enum("int32", mshadow::kInt32)
+    .add_enum("int8", mshadow::kInt8)


@reminisce will int8 cause any issue if we enable it?

Should be fine I think.

I don't think frontend support int8

ZiyueHuang · 2017-10-17T07:12:28Z

Why build fails here? Do you have any idea? @eric-haibin-lin @piiswrong

These codes can successfully compile and pass the unittest on my machine.

… into eye-csr

ZiyueHuang · 2017-10-29T03:06:49Z

Seems that tests for eye passed, CI fails due to test_maximum_minimum_scalar.

eric-haibin-lin

Some final minor comments. Should be good to merge once addressed (and if CI passes)

eric-haibin-lin · 2017-10-30T23:21:00Z

src/operator/tensor/init_op.h

+  });
+}
+
+struct eye_csr_indptr_fill {


Let's add more documentation for the new kernels

eric-haibin-lin · 2017-10-30T23:26:06Z

src/operator/tensor/init_op.h

+  }
+};
+
+struct eye_csr_data_fill {


Can we reuse this kernel instead?
https://github.com/apache/incubator-mxnet/blob/3552b958682d8ab9c3ea8e7d3f3f8855d29fbbe5/src/operator/mxnet_op.h#L295

… into eye-csr

ZiyueHuang added 5 commits October 11, 2017 23:24

eye for dense and sparse

28581cb

fix lint

f0a7cd9

Merge remote-tracking branch 'upstream/master' into eye-csr

0317a44

Fix continuous build.

gpu registration

3115556

test and fix for gpu

323647a

eric-haibin-lin reviewed Oct 12, 2017

View reviewed changes

ZiyueHuang added 3 commits October 14, 2017 19:46

address comments and add eye symbol

933d1b2

fix lint

267f689

fix for indentation

05ef6fb

eric-haibin-lin reviewed Oct 14, 2017

View reviewed changes

ZiyueHuang added 5 commits October 15, 2017 12:46

remove in8, aux_types and unneccessary import in test

cd8cca5

fix conflict in sparse.py

f787c4f

conflict in sparse.py

ff4f21f

fix conflict

05d6719

remove aux_type in ndarray.py

881326c

ZiyueHuang and others added 8 commits October 18, 2017 01:17

trigger ci

243e46a

Merge remote-tracking branch 'upstream/master' into eye-csr

7a76842

Merge branch 'master' into eye-csr

3053c24

refact eye and zeros in sparse to use same interface internally

6debc59

Merge branch 'eye-csr' of https://github.com/ZiyueHuang/incubator-mxnet…

e53dd43

… into eye-csr

comment typo

4934f7b

resolve conflict

9a889bb

Update test_sparse_ndarray.py

c5e28c5

eric-haibin-lin reviewed Oct 30, 2017

View reviewed changes

ZiyueHuang added 4 commits October 31, 2017 22:35

use set_to_int

e4c780f

Merge branch 'eye-csr' of https://github.com/ZiyueHuang/incubator-mxnet…

25e9772

… into eye-csr

Merge remote-tracking branch 'upstream/master' into eye-csr

803aa67

trigger

567d6b1

trigger CI

18a9592

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

resolve conflict

631251f

eric-haibin-lin closed this Dec 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eye for dense and sparse #8225

eye for dense and sparse #8225

ZiyueHuang commented Oct 11, 2017 •

edited

Loading

eric-haibin-lin Oct 12, 2017

eric-haibin-lin Oct 12, 2017

eric-haibin-lin Oct 12, 2017

eric-haibin-lin Oct 12, 2017

ZiyueHuang Oct 14, 2017

eric-haibin-lin Oct 12, 2017

ZiyueHuang Oct 14, 2017

eric-haibin-lin Oct 12, 2017

ZiyueHuang Oct 14, 2017 •

edited

Loading

eric-haibin-lin Oct 12, 2017

ZiyueHuang Oct 14, 2017

eric-haibin-lin Oct 12, 2017

eric-haibin-lin Oct 12, 2017

eric-haibin-lin Oct 12, 2017

eric-haibin-lin left a comment

eric-haibin-lin Oct 14, 2017

ZiyueHuang Oct 15, 2017

eric-haibin-lin Oct 14, 2017

eric-haibin-lin Oct 14, 2017 •

edited

Loading

eric-haibin-lin Oct 14, 2017

reminisce Oct 14, 2017

piiswrong Oct 14, 2017

ZiyueHuang commented Oct 17, 2017

ZiyueHuang commented Oct 29, 2017

eric-haibin-lin left a comment

eric-haibin-lin Oct 30, 2017

eric-haibin-lin Oct 30, 2017 •

edited

Loading

eye for dense and sparse #8225

eye for dense and sparse #8225

Conversation

ZiyueHuang commented Oct 11, 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

ZiyueHuang Oct 14, 2017 • 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

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

eric-haibin-lin Oct 14, 2017 • 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

ZiyueHuang commented Oct 17, 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

eric-haibin-lin Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

ZiyueHuang commented Oct 11, 2017 •

edited

Loading

ZiyueHuang Oct 14, 2017 •

edited

Loading

eric-haibin-lin Oct 14, 2017 •

edited

Loading

eric-haibin-lin Oct 30, 2017 •

edited

Loading