[GitHub] [incubator-tvm] ANSHUMAN87 commented on a change in pull request #6889: [TOPI] sparse_dense Op sparse_data input added

2020-11-19 Thread GitBox 


ANSHUMAN87 commented on a change in pull request #6889:
URL: https://github.com/apache/incubator-tvm/pull/6889#discussion_r527208388



##
File path: python/tvm/relay/op/nn/nn.py
##
@@ -1994,7 +1994,7 @@ def batch_matmul(x, y):
 
 
 # pylint: disable=no-else-return,inconsistent-return-statements
-def sparse_dense(data, weight, sparse_data=False):
+def sparse_dense(data, weight, sparse_lhs=False):
 r"""
 Computes the matrix multiplication of `data` and `weight`, where `data` is

Review comment:
   Should we change the name data & weight to dense_mat & sparse_mat ? 





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] ANSHUMAN87 commented on a change in pull request #6889: [TOPI] sparse_dense Op sparse_data input added

2020-11-18 Thread GitBox 


ANSHUMAN87 commented on a change in pull request #6889:
URL: https://github.com/apache/incubator-tvm/pull/6889#discussion_r526368800



##
File path: src/relay/op/nn/sparse.cc
##
@@ -85,10 +117,11 @@ RELAY_REGISTER_OP("nn.sparse_dense")
 )code" TVM_ADD_FILELINE)
 .set_attrs_type()
 .set_num_inputs(4)
-.add_argument("data", "nD Tensor", "Input data.")
-.add_argument("weight_data", "1D Tensor", "Weight data matrix.")
-.add_argument("weight_indices", "1D Tensor", "Weight indices matrix.")
-.add_argument("weight_indptr", "1D Tensor", "Weight indptr matrix.")
+.add_argument("input_tensor1", "nD Tensor",

Review comment:
   Okay thanks for details, I got your point clearly now! 





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] ANSHUMAN87 commented on a change in pull request #6889: [TOPI] sparse_dense Op sparse_data input added

2020-11-18 Thread GitBox 


ANSHUMAN87 commented on a change in pull request #6889:
URL: https://github.com/apache/incubator-tvm/pull/6889#discussion_r526079596



##
File path: src/relay/op/nn/sparse.cc
##
@@ -85,10 +117,11 @@ RELAY_REGISTER_OP("nn.sparse_dense")
 )code" TVM_ADD_FILELINE)
 .set_attrs_type()
 .set_num_inputs(4)
-.add_argument("data", "nD Tensor", "Input data.")
-.add_argument("weight_data", "1D Tensor", "Weight data matrix.")
-.add_argument("weight_indices", "1D Tensor", "Weight indices matrix.")
-.add_argument("weight_indptr", "1D Tensor", "Weight indptr matrix.")
+.add_argument("input_tensor1", "nD Tensor",

Review comment:
   @tkonolige : Sorry for late response. I think somehow i missed your last 
reply.
   I agree with the naming you suggested, but the issue here is one tensor 
represents 2 different use case altogether.
   For example: 
   "input_tensor2" => "weight_data_or_data_indices"
   So it can be sparse_data in one case and sparse_indices in another case.
   So i am wondering what is the best way to merge both representation ?





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] ANSHUMAN87 commented on a change in pull request #6889: [TOPI] sparse_dense Op sparse_data input added

2020-11-16 Thread GitBox 


ANSHUMAN87 commented on a change in pull request #6889:
URL: https://github.com/apache/incubator-tvm/pull/6889#discussion_r524490253



##
File path: src/relay/op/nn/sparse.cc
##
@@ -85,10 +117,11 @@ RELAY_REGISTER_OP("nn.sparse_dense")
 )code" TVM_ADD_FILELINE)
 .set_attrs_type()
 .set_num_inputs(4)
-.add_argument("data", "nD Tensor", "Input data.")
-.add_argument("weight_data", "1D Tensor", "Weight data matrix.")
-.add_argument("weight_indices", "1D Tensor", "Weight indices matrix.")
-.add_argument("weight_indptr", "1D Tensor", "Weight indptr matrix.")
+.add_argument("input_tensor1", "nD Tensor",

Review comment:
   I think we can not go back to their original names, as the tensor will 
be used for multiple purposes as given in the description string. I too am not 
impressed by the name "input_tensor1" :slightly_smiling_face: 
   Would appreciate if you can suggest some names.
   
   How about merger of both use case as below? 
   "input_tensor1"  => "data_dense_or_sparse"
   "input_tensor2"  => "weight_data_or_data_indices"
   "input_tensor3"  => "weight_indices_or_data_indptr"
   "input_tensor3"  => "weight_indptr_or_dense_weight"
   





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:
us...@infra.apache.org