[GitHub] [incubator-mxnet] mseeger commented on a change in pull request #15007: Add matrix determinant operator in linalg

[GitBox <gi...@apache.org>]

mseeger commented on a change in pull request #15007: Add matrix determinant operator in linalg
URL: https://github.com/apache/incubator-mxnet/pull/15007#discussion_r287662191
 
 

 ##########
 File path: src/operator/tensor/la_op.cc
 ##########
 @@ -939,5 +939,153 @@ NNVM_REGISTER_OP(_backward_linalg_inverse)
 .set_attr<nnvm::TIsBackward>("TIsBackward", true)
 .set_attr<FCompute>("FCompute<cpu>", LaOpBackward<cpu, 2, 2, 2, 1, inverse_backward>);
 
+NNVM_REGISTER_OP(_linalg_det)
 
 Review comment:
   Hello,
   
   I developed the linalg operators with @asmushetzel.
   
   I feel it is not very elegant to introduce a lot of MXNet operators, which are essentially just done by sticking existing operators together. It would be a lot cleaner just to provide Python functions for this (using F in {mx.nd, mx.sym} as first arg).
   
   Note we ourselves made this mistake with sumlogdiag, which is ugly and should not be there, really (we could be excused, since diag wasn't there back then).
   
   For example, if you really want logdet, you get it by a LQ decomp (linalg.geqlf), followed by log.sum over the diagonal of L, we have ops for all of this. In fact, you probably want log(abs(det)), because the determinant could be negative. You could return the sign as well.
   
   I don't understand also why a det(.) op is needed, given there is logdet(.) with sign. You can get one from the other. Also, det(.) for large matrices is prone to over or underflow anyway.
   
   It is also somewhat dangerous to offer such operators, because they end up recomputing the underlying factorizations every time. For example, to evaluate the log likelihood of a Gaussian, you need logdet and backsubstitution. You compute the Cholesky decomp. once, then use it twice. With your logdet, I cannot do that. It computes something inside, but does not return it.
   
   Finally, I also find it dangerous to offer inverse. The matrix inverse is almost never needed, but people who lack numerical maths knowledge use it. They should not, it leads to bad code. They should use matrix factorizations, like Cholesky or LQ (i.e. QR), or SVD.
   
   So, if you really want to do something useful, think about a set of Python functions for derived operators. An example:
   
   def linalg_logdet_from_chol(F, lmat):
      return F.sum(F.log(F.abs(F.diag(lmat))))
   
   # If you really want:
   def linalg_logdet(F, amat):
      return linalg_logdet_from_chol(F, F.potrf(amat))
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services