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:
[email protected]
With regards,
Apache Git Services
