ZhennanQin commented on a change in pull request #12530: Implement mkldnn
convolution fusion and quantization.
URL: https://github.com/apache/incubator-mxnet/pull/12530#discussion_r223937667
##########
File path: src/executor/graph_executor.cc
##########
@@ -1561,15 +1561,32 @@ static nnvm::Symbol PartitionGraph(const nnvm::Symbol&
src,
} else {
CHECK(i1 < arg_names.size());
CHECK_EQ(arg_names[i1], input_names[i]);
- arg_shapes.push_back(in_args[i1].shape());
- arg_dtypes.push_back(in_args[i1].dtype());
- arg_stypes.push_back(in_args[i1].storage_type());
- in_arg_ctxes[i1] = in_args[i1].ctx();
+ arg_shapes.push_back(in_args->at(i1).shape());
+ arg_dtypes.push_back(in_args->at(i1).dtype());
+ arg_stypes.push_back(in_args->at(i1).storage_type());
+ in_arg_ctxes[i1] = in_args->at(i1).ctx();
++i1;
}
}
- return PartitionGraph(src, prop_name, arg_shapes, arg_dtypes, arg_stypes,
- default_ctx, ctx_map, in_arg_ctxes, aux_state_ctxes);
+
+ // setup in_args_map
+ std::unordered_map<std::string, NDArray> in_args_map;
+ for (size_t i = 0; i < in_args->size(); ++i) {
+ in_args_map[arg_names[i]] = in_args->at(i);
Review comment:
Yeah, I understand that you can find the order from the code, but it's not
guaranteed, and you shouldn't make any assumption based on that, as it may
change in future because the order isn't a part of list_arguments() spec, at
least for now.
I guess we should answer this question first, which is, should we support
inputs with same name? If the answer is shouldn't, then we need to rename those
inputs with same name in unit test and add corresponding check and document to
disallow user using like that.
If the answer is we should, then we need to define a clear way for
distinguishing inputs apart from name on API level, instead of the undocumented
DFS order.
According to the current API design, I guess inputs with same name shouldn't
be supported, as the order of list_arguments() is unique only if inputs has
unique name. Adding DFS order to list_arguments() spec isn't user friendly, as
it's hard for end user to find out the final DFS order from a complex topology,
as single op(eg. rand_zipfian) from end-user level may be consist of many small
ops in final computing graph.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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
With regards,
Apache Git Services
