-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[Quantization]support exclude operators while quantization #15910
Conversation
@ZhennanQin @ciyongch please take a review :) |
* \param num_excluded_sym_names number of layers excluded from being quantized in the input symbol | ||
* \param excluded_sym_names node names to be excluded from being quantized | ||
* \param num_excluded_op_names number of operators excluded from being quantized in the input symbol | ||
* \param excluded_op_names operator names to be excluded from being quantized |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to use one group of the parameter to implement two functionality in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some models may define layer names with specific style. so, it's not easy to group these two functions:(
auto qnode = q_ptr(node->attrs); | ||
if (!isRegistered(qnode, dev_type)) { | ||
LOG(INFO) << "Neither FCompute nor FComputeEx registered, " << node->op()->name | ||
<< " excluded automatically."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excluded => is excluded
DFSVisit(subgraph_sym->outputs, [&](const nnvm::NodePtr& n) { | ||
if (n->is_variable()) return; | ||
if (excluded_nodes.count(n->attrs.name) || | ||
excluded_ops.count(node->op()->name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fused op is a new op, so we shouldn't check its inner node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found we cannot exclude fused conv layers when settingexcluded_op_names=['Convolution']
. Is it necessary to check the inner node here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any test case to cover subgraph ops?
https://www.tensorflow.org/performance/quantization. | ||
The calibration implementation borrows the idea of Nvidia's 8-bit Inference with TensorRT: | ||
http://on-demand.gputechconf.com/gtc/2017/presentation/s7310-8-bit-inference-with-tensorrt.pdf | ||
and adapts the method to MXNet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep this notes here for future reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file too long (>1000L):(
https://www.tensorflow.org/performance/quantization. | ||
The calibration implementation borrows the idea of Nvidia's 8-bit Inference with TensorRT: | ||
http://on-demand.gputechconf.com/gtc/2017/presentation/s7310-8-bit-inference-with-tensorrt.pdf | ||
and adapts the method to MXNet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep this notes here for future reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file too long (>1000L):(
@@ -678,6 +678,10 @@ def check_quantized_bn(data_shape, qdtype): | |||
|
|||
@with_seed() | |||
def test_quantize_params(): | |||
if is_test_for_native_cpu(): | |||
print('skipped testing quantized_pooling for native cpu since it is not supported yet') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test name? testing quantize_params
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
@ZhennanQin @ciyongch please take a final review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merging now and thanks for the contribution.
Description
Two functionally enhancement for quantization tool:
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments