[GitHub] [incubator-tvm] u99127 commented on pull request #5510: [FRONTEND][TFLite] Fully connected op conversion made in sync with TFLite

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

u99127 commented on pull request #5510:
URL: https://github.com/apache/incubator-tvm/pull/5510#issuecomment-624204486


   > 
   > 
   > > Minor nits, A better commit message would be to refer to handling batch matmul in fully connected op rather than what it stands.
   > > Isn't there a need for Quantized inputs testing here ?
   > 
   > @u99127 : Thanks for your comments! Just for your info, TFLite converter has multiple limitations in terms of various input types supported, so it is not possible always to add all the cases.
   > I had already tried for it, but converter has errors. 
   
   Thanks for trying it and clarifying the situation with qnn ops.
   
   > Also it does not support N-dim.
   > 
   > About the commit, actually the changes are quite unrelated to batch matmul, it is just that open up the issue in implementation. So IMO the commit message looks ok to me. Please let me know if you think otherwise. Thanks!
   
   
   In my experience a good git commit message, explains what was changed and why. It's ok if the body of the commit message is empty as long as the subject line can give a quick and obvious summary to the reader. 
   
   The subject line and the commit message on a quick perusal last night suggested that this commit synchronizes the implementation of fully connected with tflite. Dropping an assert always makes me query why it was dropped which is why I queried it.
   
   regards
   Ramana


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