Added the diag() operator #11643

ifeherva · 2018-07-11T13:30:48Z

Description

Added a new tensor operator called diag() replicating numpy.diag().

The only difference to numpy.diag() is that for invalid k numpy diag returns an empty array while mxnet is going to LOG FATAL.

Checklist

Essentials

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)
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Comments

This is my first operator, please be gentle :)

ifeherva · 2018-07-11T20:30:05Z

Unsure why CI fails, I did not touch that part:
/work/mxnet/3rdparty/mkldnn/install/lib//libmklml_intel.so: file not recognized: File truncated

zhreshold

Otherwise LGTM.

zhreshold · 2018-07-11T23:36:19Z

src/operator/tensor/diag_op-inl.h

+struct DiagParam : public dmlc::Parameter<DiagParam> {
+    int32_t k;
+    DMLC_DECLARE_PARAMETER(DiagParam) {
+            DMLC_DECLARE_FIELD(k)


indentation looks weird, I think the lint suggest 2 spaces or 4 spaces

zhreshold · 2018-07-11T23:47:45Z

tests/python/unittest/test_operator.py

+    r = mx.nd.diag(a)
+
+    for i in range(r.shape[0]):
+        assert r[i] == a[i][i]


you can use numpy for consistency check. It's more stable than manually setting the desired values

Good point, will do

Incorporated the changes.

ifeherva · 2018-07-13T00:59:25Z

Any idea why it does not pass CI?

zhreshold · 2018-07-13T01:03:30Z

@ifeherva Try locally simulate the CI with the docker build script. Currently we can get nothing from the log.

ifeherva · 2018-07-13T19:53:29Z

Ran on my p3.2x.large

python3 ci/build.py --platform ubuntu_build_cuda /work/runtime_functions.sh build_ubuntu_gpu_cuda91_cudnn7

without issues

ifeherva · 2018-07-18T16:27:07Z

Operator is now passing all tests, I consider this work finished.

szha · 2018-07-18T18:31:33Z

@eric-haibin-lin seems like there should be a sparse version for this too. Thoughts?

zhreshold · 2018-07-18T18:33:31Z

LGTM now. @eric-haibin-lin Can you do a quick review?

eric-haibin-lin · 2018-07-18T18:47:00Z

python/mxnet/ndarray/ndarray.py

@@ -1302,6 +1302,14 @@ def flip(self, *args, **kwargs):
        """
        return op.flip(self, *args, **kwargs)

+    def diag(self, k=0, **kwargs):


Please also update https://github.com/apache/incubator-mxnet/blob/master/docs/api/python/ndarray/ndarray.md and symbol/symbol.md (probably add it to the section of array creation routines like https://docs.scipy.org/doc/numpy/reference/routines.array-creation.html)
@reminisce maybe we should also mention adding documentation in the operator tutorial ?

Good point, added it.

eric-haibin-lin · 2018-07-18T18:54:00Z

@ifeherva thanks for the contribution! Good work.

@szha I think sparse.diag which returns a CSR ndarray (like https://docs.scipy.org/doc/scipy-0.14.0/reference/generated/scipy.sparse.diags.html) is a good thing to have. It will be great if we can have that, but if not, this PR shall not be block by this extra feature

szha · 2018-07-18T18:56:01Z

@eric-haibin-lin Let's have the sparse feature in a separate PR.

ifeherva · 2018-07-18T19:39:25Z

I am happy to do the sparse array support in a future PR.

Done: 2d input forward pass Missing: 1d input forward all backward

Fixed small typos as well

Finished function documentation Added unit tests

Issues were extra white spaces and include order

Added matching test case

eric-haibin-lin

LGTM pending one minor comment for doc

eric-haibin-lin · 2018-07-19T16:49:36Z

docs/api/python/ndarray/ndarray.md

@@ -131,6 +131,7 @@ The `ndarray` package provides several classes:
    NDArray.flatten
    NDArray.expand_dims
    NDArray.split
+    NDArray.diag
 ```


Sorry I didn't make it clear - there're two places to add per file. For ndarray.md, One is NDArray.diag (fluent method) and the other is (ndarray.)diag at line 360.

* Added np.diag as mxnet operator, WIP Done: 2d input forward pass Missing: 1d input forward all backward * Added a simple gradient transfer backwards operator for diag Fixed small typos as well * Finished backward operation * Added full support for k * Finished added the 1D case to the diag operator Finished function documentation Added unit tests * Fixed cpplinter errors in the diag operator Issues were extra white spaces and include order * Fixed indentation in diag_op-inl.h * Changed diag operator tests to use np.diag() as comparison * Fixed kernel bug in gpu diag operator * Replaced the min operator with an inline if statement. * Added diag to ndarray and symbol * Replaced the type of parameter k from int32 to nnvm::dim * Added default argument to k in ndarray and symbol * Fixed ndarray and symbol diag calls * Fixed the optional k parameter * Fixed cpp linting error * Changed test data datatype to float32 * K values resulting into 0-sized diagonals will now throw an exception. Added matching test case * Fixed unittest * Added diag to NDArray and Symbol api doc * Added missing api doc

ifeherva requested a review from anirudh2290 as a code owner July 11, 2018 13:30

ifeherva mentioned this pull request Jul 11, 2018

Numpy Diag in MXNet #9253

Closed

ifeherva requested a review from szha as a code owner July 11, 2018 21:11

ifeherva force-pushed the diagonal_operator branch 2 times, most recently from 66f6335 to 1bca54a Compare July 11, 2018 21:22

zhreshold suggested changes Jul 11, 2018

View reviewed changes

ifeherva force-pushed the diagonal_operator branch 2 times, most recently from b764ef9 to d2ef62e Compare July 13, 2018 00:17

zhreshold approved these changes Jul 13, 2018

View reviewed changes

ifeherva force-pushed the diagonal_operator branch 5 times, most recently from 871f510 to f54b7ac Compare July 17, 2018 05:17

szha requested a review from eric-haibin-lin July 18, 2018 18:30

zhreshold approved these changes Jul 18, 2018

View reviewed changes

eric-haibin-lin reviewed Jul 18, 2018

View reviewed changes

ifeherva added 3 commits July 18, 2018 17:33

Added np.diag as mxnet operator, WIP

157e3fe

Done: 2d input forward pass Missing: 1d input forward all backward

Added a simple gradient transfer backwards operator for diag

82ff682

Fixed small typos as well

Finished backward operation

182f11b

ifeherva added 17 commits July 18, 2018 17:33

Added full support for k

0581d45

Finished added the 1D case to the diag operator

eb5f17a

Finished function documentation Added unit tests

Fixed cpplinter errors in the diag operator

cb89c76

Issues were extra white spaces and include order

Fixed indentation in diag_op-inl.h

95b5560

Changed diag operator tests to use np.diag() as comparison

a291e81

Fixed kernel bug in gpu diag operator

bb4c8d1

Replaced the min operator with an inline if statement.

7131e72

Added diag to ndarray and symbol

ac7feba

Replaced the type of parameter k from int32 to nnvm::dim

0b0fe12

Added default argument to k in ndarray and symbol

57b9345

Fixed ndarray and symbol diag calls

6b99f45

Fixed the optional k parameter

a613f49

Fixed cpp linting error

869e198

Changed test data datatype to float32

d02e221

K values resulting into 0-sized diagonals will now throw an exception.

17cdcf9

Added matching test case

Fixed unittest

fc2a2ca

Added diag to NDArray and Symbol api doc

702b416

ifeherva force-pushed the diagonal_operator branch from a57b8d5 to 702b416 Compare July 19, 2018 00:34

eric-haibin-lin approved these changes Jul 19, 2018

View reviewed changes

Added missing api doc

7fd3b8c

eric-haibin-lin merged commit f15b1b8 into apache:master Jul 19, 2018

ifeherva deleted the diagonal_operator branch February 10, 2019 04:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the diag() operator #11643

Added the diag() operator #11643

ifeherva commented Jul 11, 2018 •

edited

Loading

ifeherva commented Jul 11, 2018

zhreshold left a comment

zhreshold Jul 11, 2018

zhreshold Jul 11, 2018

ifeherva Jul 11, 2018

ifeherva Jul 12, 2018

ifeherva commented Jul 13, 2018

zhreshold commented Jul 13, 2018

ifeherva commented Jul 13, 2018

ifeherva commented Jul 18, 2018

szha commented Jul 18, 2018

zhreshold commented Jul 18, 2018

eric-haibin-lin Jul 18, 2018

reminisce Jul 18, 2018

ifeherva Jul 18, 2018

eric-haibin-lin commented Jul 18, 2018

szha commented Jul 18, 2018

ifeherva commented Jul 18, 2018

eric-haibin-lin left a comment

eric-haibin-lin Jul 19, 2018

ifeherva Jul 19, 2018

Added the diag() operator #11643

Added the diag() operator #11643

Conversation

ifeherva commented Jul 11, 2018 • edited Loading

Description

Checklist

Essentials

Comments

ifeherva commented Jul 11, 2018

zhreshold 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

ifeherva commented Jul 13, 2018

zhreshold commented Jul 13, 2018

ifeherva commented Jul 13, 2018

ifeherva commented Jul 18, 2018

szha commented Jul 18, 2018

zhreshold commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-haibin-lin commented Jul 18, 2018

szha commented Jul 18, 2018

ifeherva commented Jul 18, 2018

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

ifeherva commented Jul 11, 2018 •

edited

Loading