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

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

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

 ##########
 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:
   I'm not convinced that we should add all types of variants like "det", "logdet" etc. Conceptually there is no end to that (why not have logdetminus1?). It also doesn't save any compute time to provide as the overwhelming compute is in the core determinant computation and not in a subsequent log or exp. And we end up with a lot of duplicate redundant code. 
   
   So I would propose to provide one generic method, which is likely signed logdet() and leave it to the user to apply additional operators when she/he needs some variant. You can even make the return of "sign" optional by a parameter.  

----------------------------------------------------------------
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