[GitHub] [incubator-mxnet] HahTK commented on a change in pull request #16131: Fix for duplicate subgraph inputs/outputs

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

HahTK commented on a change in pull request #16131: Fix for duplicate subgraph inputs/outputs
URL: https://github.com/apache/incubator-mxnet/pull/16131#discussion_r323521071
 
 

 ##########
 File path: src/operator/subgraph/subgraph_property.h
 ##########
 @@ -296,8 +296,20 @@ class SubgraphProperty {
    */
   virtual void ConnectSubgraphOutputs(const nnvm::NodePtr subgraph_node,
                                       std::vector<nnvm::NodeEntry*>* output_entries) const {
+    // Collapse output_entries pointing to same NodeEntry
 
 Review comment:
   It is important to be clear about 2 things :
   1. "output_entries" are misnamed. They are really input_entries_consuming_subgraph_output.
   2. Therefore, output_entries do not have duplicate entries. The number of output_entries is correct ! (or rather the the number of input_entries_consuming_subgraph_output is correct)
   
   Consider the following 3 node json example -
   (json simplified to eliminate irrelevant fields)
   
   ```
   nodes : [ 
     { name : "NodeA" }, #nodes[0]
     { name : "NodeB",   #nodes[1]
       inputs : [[0,0,0]] }, #pointing at NodeA 
     { name : "NodeC",  #nodes[2]
       inputs : [[0,0,0]] } #pointing at NodeA 
   ]
   ```
   
   Suppose NodeA gets turned into a subgraph with 1 node. If we do that then, 
   The so called "output_entries" are :
   1. nodes[1][inputs][0] #0th input of NodeB
   2. nodes[2][inputs[0] #0th input of NodeC
   
   Therefore the variable names "output_entries" is actually refering to the the "input_entries_consuming_subgraph_output". 
   The correct number of "output_entries" is therefore 2. 
   We can not collapse this down to 1 because both NodeB and NodeC need to point back to NodeA
   
   The real outputs are "sym.outputs" in the build_graph.cc code in the PR. 
   We do de-duplicate those.
   

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