[GitHub] apeforest commented on a change in pull request #13000: fix quantize_graph pass error when there're multiple outputs from a single node
apeforest commented on a change in pull request #13000: fix quantize_graph pass
error when there're multiple outputs from a single node
URL: https://github.com/apache/incubator-mxnet/pull/13000#discussion_r235527481
##
File path: src/operator/quantization/quantize_graph_pass.cc
##
@@ -156,29 +158,54 @@ Graph QuantizeGraph(Graph &) {
new_node->inputs.emplace_back(mirror_entry);
} else if (!NeedQuantize(e.node, excluded_nodes) &&
(mirror_node->op() == nullptr ||
-mirror_node->op()->name != "_contrib_quantize")) {
+mirror_node->op()->name != "_contrib_quantize") &&
Review comment:
Using `entry` is fine with me as long as you are not only comparing the op
name
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
[GitHub] apeforest commented on a change in pull request #13000: fix quantize_graph pass error when there're multiple outputs from a single node
apeforest commented on a change in pull request #13000: fix quantize_graph pass
error when there're multiple outputs from a single node
URL: https://github.com/apache/incubator-mxnet/pull/13000#discussion_r235527167
##
File path: src/operator/quantization/quantize_graph_pass.cc
##
@@ -117,9 +117,10 @@ inline bool NeedQuantize(NodePtr node, const
std::unordered_set& ex
}
Graph QuantizeGraph(Graph &) {
- static auto& quantized_op_map =
Op::GetAttr("FQuantizedOp");
- static auto& need_requantize_map =
Op::GetAttr("FNeedRequantize");
- static auto& avoid_quantize_input_map =
+ static const auto& flist_outputs =
nnvm::Op::GetAttr("FListOutputNames");
Review comment:
If so, keeping it consistent with other places is fine with me.
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
[GitHub] apeforest commented on a change in pull request #13000: fix quantize_graph pass error when there're multiple outputs from a single node
apeforest commented on a change in pull request #13000: fix quantize_graph pass
error when there're multiple outputs from a single node
URL: https://github.com/apache/incubator-mxnet/pull/13000#discussion_r235527167
##
File path: src/operator/quantization/quantize_graph_pass.cc
##
@@ -117,9 +117,10 @@ inline bool NeedQuantize(NodePtr node, const
std::unordered_set& ex
}
Graph QuantizeGraph(Graph &) {
- static auto& quantized_op_map =
Op::GetAttr("FQuantizedOp");
- static auto& need_requantize_map =
Op::GetAttr("FNeedRequantize");
- static auto& avoid_quantize_input_map =
+ static const auto& flist_outputs =
nnvm::Op::GetAttr("FListOutputNames");
Review comment:
If so, keeping it is fine with me as long as it is consistent.
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
[GitHub] apeforest commented on a change in pull request #13000: fix quantize_graph pass error when there're multiple outputs from a single node
apeforest commented on a change in pull request #13000: fix quantize_graph pass
error when there're multiple outputs from a single node
URL: https://github.com/apache/incubator-mxnet/pull/13000#discussion_r235526884
##
File path: src/operator/quantization/quantize_graph_pass.cc
##
@@ -156,29 +158,54 @@ Graph QuantizeGraph(Graph &) {
new_node->inputs.emplace_back(mirror_entry);
} else if (!NeedQuantize(e.node, excluded_nodes) &&
(mirror_node->op() == nullptr ||
-mirror_node->op()->name != "_contrib_quantize")) {
+mirror_node->op()->name != "_contrib_quantize") &&
Review comment:
Checking `entry` as you suggested is fine. Checking op name with a string
seems to be error-prone and requires many updates if we later change the op name
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
[GitHub] apeforest commented on a change in pull request #13000: fix quantize_graph pass error when there're multiple outputs from a single node
apeforest commented on a change in pull request #13000: fix quantize_graph pass
error when there're multiple outputs from a single node
URL: https://github.com/apache/incubator-mxnet/pull/13000#discussion_r235220673
##
File path: src/operator/quantization/quantize_graph_pass.cc
##
@@ -215,12 +242,19 @@ Graph QuantizeGraph(Graph &) {
min_index = num_outputs + 2 * e.index;
max_index = num_outputs + 2 * e.index + 1;
} else {
- CHECK(mirror_node->op() != nullptr &&
-mirror_node->op()->name == "_contrib_quantize")
+ CHECK((mirror_node->op() != nullptr &&
+mirror_node->op()->name == "_contrib_quantize") ||
Review comment:
See reply above
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
[GitHub] apeforest commented on a change in pull request #13000: fix quantize_graph pass error when there're multiple outputs from a single node
apeforest commented on a change in pull request #13000: fix quantize_graph pass
error when there're multiple outputs from a single node
URL: https://github.com/apache/incubator-mxnet/pull/13000#discussion_r235220549
##
File path: src/operator/quantization/quantize_graph_pass.cc
##
@@ -156,29 +158,54 @@ Graph QuantizeGraph(Graph &) {
new_node->inputs.emplace_back(mirror_entry);
} else if (!NeedQuantize(e.node, excluded_nodes) &&
(mirror_node->op() == nullptr ||
-mirror_node->op()->name != "_contrib_quantize")) {
+mirror_node->op()->name != "_contrib_quantize") &&
Review comment:
```suggestion
mirror_node->op() != nnvm::Op::Get("_contrib_quantize")
```
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
[GitHub] apeforest commented on a change in pull request #13000: fix quantize_graph pass error when there're multiple outputs from a single node
apeforest commented on a change in pull request #13000: fix quantize_graph pass
error when there're multiple outputs from a single node
URL: https://github.com/apache/incubator-mxnet/pull/13000#discussion_r235220225
##
File path: src/operator/quantization/quantize_graph_pass.cc
##
@@ -156,29 +158,54 @@ Graph QuantizeGraph(Graph &) {
new_node->inputs.emplace_back(mirror_entry);
} else if (!NeedQuantize(e.node, excluded_nodes) &&
(mirror_node->op() == nullptr ||
-mirror_node->op()->name != "_contrib_quantize")) {
+mirror_node->op()->name != "_contrib_quantize") &&
Review comment:
Comparing to a plain string is error prone. What if someone mis-spelled by
one char. It's going to be hard to debug.
In fact, I think a more rigorous way to check if an operator may be checking
the operator itself: if (op == nnvm::Op::Get("_contrib_quantize"))
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
[GitHub] apeforest commented on a change in pull request #13000: fix quantize_graph pass error when there're multiple outputs from a single node
apeforest commented on a change in pull request #13000: fix quantize_graph pass
error when there're multiple outputs from a single node
URL: https://github.com/apache/incubator-mxnet/pull/13000#discussion_r234396815
##
File path: src/operator/quantization/quantize_graph_pass.cc
##
@@ -156,29 +158,54 @@ Graph QuantizeGraph(Graph &) {
new_node->inputs.emplace_back(mirror_entry);
} else if (!NeedQuantize(e.node, excluded_nodes) &&
(mirror_node->op() == nullptr ||
-mirror_node->op()->name != "_contrib_quantize")) {
+mirror_node->op()->name != "_contrib_quantize") &&
Review comment:
Make _contrib_quantize a constant variable. Or even better this should be a
function that returns op name
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
[GitHub] apeforest commented on a change in pull request #13000: fix quantize_graph pass error when there're multiple outputs from a single node
apeforest commented on a change in pull request #13000: fix quantize_graph pass
error when there're multiple outputs from a single node
URL: https://github.com/apache/incubator-mxnet/pull/13000#discussion_r234396759
##
File path: src/operator/quantization/quantize_graph_pass.cc
##
@@ -117,9 +117,10 @@ inline bool NeedQuantize(NodePtr node, const
std::unordered_set& ex
}
Graph QuantizeGraph(Graph &) {
- static auto& quantized_op_map =
Op::GetAttr("FQuantizedOp");
- static auto& need_requantize_map =
Op::GetAttr("FNeedRequantize");
- static auto& avoid_quantize_input_map =
+ static const auto& flist_outputs =
nnvm::Op::GetAttr("FListOutputNames");
Review comment:
nit: output_list is a better
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
[GitHub] apeforest commented on a change in pull request #13000: fix quantize_graph pass error when there're multiple outputs from a single node
apeforest commented on a change in pull request #13000: fix quantize_graph pass
error when there're multiple outputs from a single node
URL: https://github.com/apache/incubator-mxnet/pull/13000#discussion_r234396881
##
File path: src/operator/quantization/quantize_graph_pass.cc
##
@@ -215,12 +242,19 @@ Graph QuantizeGraph(Graph &) {
min_index = num_outputs + 2 * e.index;
max_index = num_outputs + 2 * e.index + 1;
} else {
- CHECK(mirror_node->op() != nullptr &&
-mirror_node->op()->name == "_contrib_quantize")
+ CHECK((mirror_node->op() != nullptr &&
+mirror_node->op()->name == "_contrib_quantize") ||
Review comment:
Same here replace the op name by a function
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
[GitHub] apeforest commented on a change in pull request #13000: fix quantize_graph pass error when there're multiple outputs from a single node
apeforest commented on a change in pull request #13000: fix quantize_graph pass
error when there're multiple outputs from a single node
URL: https://github.com/apache/incubator-mxnet/pull/13000#discussion_r234396859
##
File path: src/operator/quantization/quantize_graph_pass.cc
##
@@ -130,6 +131,7 @@ Graph QuantizeGraph(Graph &) {
// graph. Key is the currently visited graph's node pointer, and value is a
copied node of the key
// node. The existing key's value may be updated with the newly created
quantize/dequantize op.
std::unordered_map mirror_map;
+ nnvm::NodeEntryMap multiple_outputs_entry_map;
Review comment:
I would prefer a generic way to handle both single and multiple outputs than
treating them differently
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
[GitHub] apeforest commented on a change in pull request #13000: fix quantize_graph pass error when there're multiple outputs from a single node
apeforest commented on a change in pull request #13000: fix quantize_graph pass
error when there're multiple outputs from a single node
URL: https://github.com/apache/incubator-mxnet/pull/13000#discussion_r234396729
##
File path: src/operator/quantization/quantize_graph_pass.cc
##
@@ -156,29 +158,54 @@ Graph QuantizeGraph(Graph &) {
new_node->inputs.emplace_back(mirror_entry);
} else if (!NeedQuantize(e.node, excluded_nodes) &&
(mirror_node->op() == nullptr ||
-mirror_node->op()->name != "_contrib_quantize")) {
+mirror_node->op()->name != "_contrib_quantize") &&
+(!multiple_outputs_entry_map.count(e))) {
+ // When there're multiple entrys outgoing from a single node, need
to add entry
+ // index (or output name) into quantize/min/max node to distinguish
them.
+ // Or the output name is not ending with 'output', just put the
output name here
+ // to better align with calibration phase. No need to change name to
weights/bias.
+ std::string subfix = "";
Review comment:
Do you mean suffix?
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
