New operators linalg_syrk, linalg_gelqf #7741

mseeger · 2017-09-05T12:54:22Z

Works for CPU only for now. We will supply the GPU versions next
Added dtype parameters to unit test support code, so we can run numerical tests in float64

asmushetzel · 2017-09-05T15:18:31Z

src/operator/tensor/la_op.cc


 Examples::

   // Single matrix multiply
   A = [[1.0, 1.0], [1.0, 1.0]]
   B = [[1.0, 1.0], [1.0, 1.0], [1.0, 1.0]]
-   gemm2(A, B, transpose_b = 1, alpha = 2.0)


Apparently some general renaming has happened recently for namespace reasons. All documentation now uses function names without linalg_prefix.

OK, I will change the docstrings

asmushetzel · 2017-09-05T15:20:02Z

src/operator/tensor/la_op.cc

@@ -251,42 +253,42 @@ NNVM_REGISTER_OP(_backward_linalg_potri)
 .set_attr<nnvm::TIsBackward>("TIsBackward", true)
 .set_attr<FCompute>("FCompute<cpu>", LaOpBackward<cpu, 2, 2, 3, 1, potri_backward>);

-NNVM_REGISTER_OP(_linalg_trmm)
+NNVM_REGISTER_OP(linalg_trmm)


All these operators are now registered with an initial underscore followed by an alias for backward compatibility.

OK, I will do this

asmushetzel · 2017-09-05T15:21:21Z

src/operator/tensor/la_op.cc

@@ -420,5 +421,134 @@ NNVM_REGISTER_OP(_backward_linalg_sumlogdiag)
 .set_attr<nnvm::TIsBackward>("TIsBackward", true)
 .set_attr<FCompute>("FCompute<cpu>", LaOpBackward<cpu, 2, 2, 2, 1, sumlogdiag_backward>);

+NNVM_REGISTER_OP(linalg_syrk)


Initial underscore followed by alias (as well as in the other new operators)

OK, will do this

asmushetzel · 2017-09-05T15:34:21Z

src/operator/tensor/la_op.h

-  if ( ndim < dim ) {
-     return false;
-  }
+  CHECK_GE(ndim, dim) << "Shape of input has too few dimensions";


Please leave original code. We may have ndim < dim in the case where the input shape has not yet been defined at all (but may be defined later). Means at this point in time we can't do any inference but not necessary that we can't ever (in a subsequent pass).

asmushetzel · 2017-09-06T09:25:15Z

src/operator/linalg.h

+// CPU/GPU-versions of LAPACK functions "gelqf", "orglq". Please refer to the
+// LAPACK documentation for further details.
+// Note:
+// - The current implementation works for CPU only. In particular, when called


There are no separate batch mode functions for this. So we should not refer to batch mode functions in this comment or add batch mode functions.

piiswrong · 2017-09-06T17:34:06Z

docs/api/python/symbol.md

@@ -526,6 +526,8 @@ Composite multiple symbols into a new one by an operator.
    linalg_trmm
    linalg_trsm
    linalg_sumlogdiag
+	linalg_syrk


wrong indentation

piiswrong · 2017-09-06T17:35:26Z

what's the status on this?

… in float64 now

mseeger · 2017-09-06T18:44:11Z

I managed to run all unit tests in continuous-integration/jenkins/pr-head, except for the R ones. They fail for funny reasons, which seem nothing to do with my changes.

I am trying again.

mseeger · 2017-09-06T19:20:24Z

Unit tests for esoteric APIs (Perl, R, Scala) fail with:

Remote call on mxnet14 failed

I fail to see what this has to do with my code changes. Not interested in these APIs (or should I be?).

asmushetzel · 2017-09-06T20:03:02Z

The R/Perl-unit tests were failing because of a git-issue. All other ones did pass. I did a full review and IMO all is fine now.

There will be a followup PR by me in a week or so that will bring in the CUDA support for these new operators.

So from my point of view, this can be integrated. Just would like to have confirmation from Matthias that the documentation changes look as expected when formatted.

mseeger · 2017-09-06T20:05:08Z

Yes I confirmed all docstrings in the tool you sent me, they all look fine.

asmushetzel · 2017-09-06T20:57:22Z

Thank you Eric for the incredible turnaround time

eric-haibin-lin · 2017-09-06T21:17:45Z

docs/api/python/symbol.md

@@ -526,6 +526,8 @@ Composite multiple symbols into a new one by an operator.
    linalg_trmm
    linalg_trsm
    linalg_sumlogdiag
+    linalg_syrk
+    linalg_gelqf


I assume ndarray.md should be updated, too?

iblislin · 2017-09-07T15:45:43Z

src/operator/linalg_impl.h

+                             const Tensor<cpu, 2, DType>& B, DType alpha, \
+                             DType beta, bool tA, Stream<cpu> *s) { \
+  check_syrk(A, B, alpha, beta, tA); \
+  cblas_##fname(CblasRowMajor, CblasLower, (tA ? CblasTrans : CblasNoTrans), \


Hi, MXNet.jl got compilation error when build from MXNet's master:
https://travis-ci.org/dmlc/MXNet.jl/jobs/272939237#L1213

any idea?

src/operator/contrib/./../linalg_impl.h:782:1: note: in expansion of macro ‘LINALG_CPU_SYRK’ LINALG_CPU_SYRK(ssyrk, float) ^ src/operator/contrib/./../linalg_impl.h: In function ‘void linalg_syrk(const mshadow::Tensor<Device, 2, DType>&, const mshadow::Tensor<Device, 2, DType>&, DType, DType, bool, mshadow::Stream<Device>*) [with xpu = mshadow::cpu; DType = double]’: src/operator/contrib/./../linalg_impl.h:748:61: error: ‘cblas_dsyrk’ was not declared in this scope A.dptr_, A.stride_, beta, B.dptr_, B.stride_); \ ^ src/operator/contrib/./../linalg_impl.h:783:1: note: in expansion of macro ‘LINALG_CPU_SYRK’ LINALG_CPU_SYRK(dsyrk, double) ^

Hello, {s|d}syrk is a BLAS function, which can be called as cblas_{s|d}syrk (via the cblas interface).
For whatever reason, this does not work in your particular setup. What is quite odd about this is that linalg_impl.h calls a range of other cblas_XXX functions:
cblas_*gemm, cblas_*trmm, cblas_*trsm (where * in s, d).

AFAIK, cblas has all of these AND cblas_*syrk.

Are you getting errors for the other cblas calls as well?

Maybe @asmushetzel has an idea what is going on here?

I finally figured out that MXNet.jl needs a modified cblas.h: https://github.com/dmlc/MXNet.jl/blob/master/deps/cblas.h

I will manage to send a patch for that. @mseeger thanks for your time and explanation!

It seems that your cblas.h is edited, right? This is not good. Maybe find out what Julia is doing there?
We may rely on further blas functions in the future, so better try and get the correct cblas.h being used

… in float64 now (apache#7741)

asmushetzel · 2017-09-08T07:47:18Z

Looks as if we don't have testing Julia-build on Jenkins. That's why such things slip through.
Apparently the Julia-build maintains its ow hand-written cblas-include file.
Chris, can you drive sufficient automatic testing for this build?

mseeger · 2017-09-08T08:50:47Z

This was also my thought. Why do they edit cblas.h? Just to save some compile time?

… in float64 now (apache#7741)

asmushetzel reviewed Sep 5, 2017

View reviewed changes

asmushetzel reviewed Sep 6, 2017

View reviewed changes

piiswrong reviewed Sep 6, 2017

View reviewed changes

New operators linalg_syrk, linalg_gelqf. Numerical unit tests can run…

e9ab7f7

… in float64 now

piiswrong merged commit 541158d into apache:master Sep 6, 2017

eric-haibin-lin reviewed Sep 6, 2017

View reviewed changes

mseeger deleted the mseeger-linalg-newops branch September 7, 2017 07:42

iblislin reviewed Sep 7, 2017

View reviewed changes

iblislin mentioned this pull request Sep 7, 2017

base/build: using Libdl.dlext for searching lib dmlc/MXNet.jl#265

Merged

cjolivier01 pushed a commit to cjolivier01/mxnet that referenced this pull request Sep 7, 2017

New operators linalg_syrk, linalg_gelqf. Numerical unit tests can run…

c042b14

… in float64 now (apache#7741)

iblislin mentioned this pull request Sep 8, 2017

cblas: import all func prototype into header dmlc/MXNet.jl#267

Closed

cjolivier01 pushed a commit to cjolivier01/mxnet that referenced this pull request Sep 11, 2017

New operators linalg_syrk, linalg_gelqf. Numerical unit tests can run…

b021bc6

… in float64 now (apache#7741)

mbaijal pushed a commit to mbaijal/incubator-mxnet that referenced this pull request Sep 19, 2017

New operators linalg_syrk, linalg_gelqf. Numerical unit tests can run…

73bbf31

… in float64 now (apache#7741)

mbaijal pushed a commit to mbaijal/incubator-mxnet that referenced this pull request Sep 19, 2017

New operators linalg_syrk, linalg_gelqf. Numerical unit tests can run…

5a00bf5

… in float64 now (apache#7741)

mbaijal pushed a commit to mbaijal/incubator-mxnet that referenced this pull request Sep 20, 2017

New operators linalg_syrk, linalg_gelqf. Numerical unit tests can run…

3ac5d89

… in float64 now (apache#7741)

iblislin mentioned this pull request Oct 8, 2017

WIP: Julia CI build #8175

Merged

crazy-cat pushed a commit to crazy-cat/incubator-mxnet that referenced this pull request Oct 26, 2017

New operators linalg_syrk, linalg_gelqf. Numerical unit tests can run…

b06fd46

… in float64 now (apache#7741)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New operators linalg_syrk, linalg_gelqf #7741

New operators linalg_syrk, linalg_gelqf #7741

mseeger commented Sep 5, 2017

asmushetzel Sep 5, 2017

mseeger Sep 5, 2017

asmushetzel Sep 5, 2017

mseeger Sep 5, 2017

asmushetzel Sep 5, 2017

mseeger Sep 5, 2017

asmushetzel Sep 5, 2017

asmushetzel Sep 6, 2017

piiswrong Sep 6, 2017

mseeger Sep 6, 2017

piiswrong commented Sep 6, 2017

mseeger commented Sep 6, 2017

mseeger commented Sep 6, 2017

asmushetzel commented Sep 6, 2017

mseeger commented Sep 6, 2017

asmushetzel commented Sep 6, 2017

eric-haibin-lin Sep 6, 2017

iblislin Sep 7, 2017

mseeger Sep 7, 2017

iblislin Sep 8, 2017 •

edited

Loading

mseeger Sep 8, 2017

asmushetzel commented Sep 8, 2017 •

edited

Loading

mseeger commented Sep 8, 2017

New operators linalg_syrk, linalg_gelqf #7741

New operators linalg_syrk, linalg_gelqf #7741

Conversation

mseeger commented Sep 5, 2017

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 Sep 6, 2017

mseeger commented Sep 6, 2017

mseeger commented Sep 6, 2017

asmushetzel commented Sep 6, 2017

mseeger commented Sep 6, 2017

asmushetzel commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iblislin Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmushetzel commented Sep 8, 2017 • edited Loading

mseeger commented Sep 8, 2017

iblislin Sep 8, 2017 •

edited

Loading

asmushetzel commented Sep 8, 2017 •

edited

Loading