-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add test for the qnn_add operator #4282
Conversation
The tests use fake quant approach so until the tf session tensors remain in float32. The test data has to be passed in uint8 because of how the tflite/tvm comparison works. Abs tolerance up to 1 is allowed for the qnn results. For now input_stats are hardcoded assuming the tests for the other qnn ops will pass the input data in the same range.
@FrozenGene @anijain2305 can you please review this patch |
@inadob Thanks for improvement. Abs tolerance up to 1 doesn't make sense. I think it should be the problem I discuss with @anijain2305 : #3900 (comment). Better way should be we handle it in TFLite parser, then we could compare it with tflite elementwise, you don't need to set it abs tolerance up to 1, it is too large. I could do it later. Or if you are interested, you could handle this issue, how about you? |
A test for qnn_mul has to be added when the qnn elemwise tests (apache#4282) get merged.
If I understand tolerance correctly, I think atol of 1 is good for integers and current implementation. Let me know if I am wrong. For integers, either you say match exactly, or if you want to have a tolerance, it can be minimum 1. There is nothing in between. Currently, as there is a difference between TFLite and TVM rounding, there is bound to be some differences and the minimum difference can be 1. This PR makes a big jump in improving the testing infrastructure for TFLite quantized networks and enabling unittests. So, I think this is a very good addition. The atol is an artifact of implementation which we can solve separately. @FrozenGene Will you be willing to create a separate RFC for implementing TFLite rounding in TVM? I don't think we should stall this PR. |
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.
Overall LGTM. Thanks for this. Any way to improve the testing infrastructure is a big help in TVM. So really thanks for this :)
@anijain2305 I only concerned we will let our wrong implementation off. For example, we implement one operator XXX, we follow the test infra setting abs tolerance be 1. However, our implementation of XXX is wrong and only different 1 between correct result. This is my real concern before, not pointing to this pr. I would like to implement TFLite rounding next and wish we could change the tolerance after that. We could let this PR in firstly :-) |
Isolate qnn uint8 elemwise tests Remove blank lines
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. @zhiics Can you please review and assign?
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
Thanks @inadob @anijain2305 @FrozenGene |
* Add test for the qnn_add operator The tests use fake quant approach so until the tf session tensors remain in float32. The test data has to be passed in uint8 because of how the tflite/tvm comparison works. Abs tolerance up to 1 is allowed for the qnn results. For now input_stats are hardcoded assuming the tests for the other qnn ops will pass the input data in the same range. * Separate qnn uint8 test function from the fp32 elemwise tests Isolate qnn uint8 elemwise tests Remove blank lines
A test for qnn_mul has to be added when the qnn elemwise tests (#4282) get merged.
A test for qnn_mul has to be added when the qnn elemwise tests (apache#4282) get merged.
A test for qnn_mul has to be added when the qnn elemwise tests (apache#4282) get merged.
* [TOPI][OP] Support Faster-RCNN Proposal OP on CPU (apache#4297) * Support Proposal operator on CPU. * PyLint space issue * PyLint space issue * Pylint singleton-comparison issue * [QNN][Legalize] Specialize for Platforms without any fast Int8 arithmetic units. (apache#4307) * fix error when memory_id is VTA_MEM_ID_OUT (apache#4330) * [CI][DOCKER] Add ONNX runtime dep (apache#4314) * [DOCKER] Add ONNX runtime dep * Improve ci script * [QNN] Quantize - Fixing the sequence of lowering. (apache#4316) * [QNN] Use Int16 upcast in Fallback Conv2D. Fix test names. (apache#4329) * [doc][fix] fix sphinx parsing for pass infra tutorial (apache#4337) * change ci image version (apache#4313) * [Codegen] remove fp16 function override for cuda (apache#4331) * add volatile override back * [codegen] remove fp16 function override for cuda * [CI] Set workspace to be per executor (apache#4336) * [Build][Windows] Fix Windows build by including cctype (apache#4319) * Fix build * dummy change to retrigger CI * dummy change to retrigger ci * dummy change to retrigger ci * Enable hipModuleGetGlobal() (apache#4321) * [Relay][Pass] Add pass to remove unused functions in relay module (apache#4334) * [Relay][Pass] Add pass to remove unused functions in relay module * Add tests * Fix lint * Fix visit order * Add pass argument * Fix * Add support for quant. mul operator in tflite frontend (apache#4283) A test for qnn_mul has to be added when the qnn elemwise tests (apache#4282) get merged. * Add topi.nn.fifo_buffer to TVM doc (apache#4343) * Solve custom model of prelu (apache#4326) * Deprecate NNVM warning msg (apache#4333) * [Contrib] Add MKL DNN option (apache#4323) * [Contrib] Add MKL DNN * update * update * [Relay][Frontend][TF] Fix transpose when axes is not a param (apache#4327) * [Relay][Frontend][TF] Use _infer_value_simulated when axes is not a const to Transpose * uncomment tests * dummy change to retrigger ci * [RUNTIME] Add device query for AMD GcnArch (apache#4341) * add gcnArch query * kGcnArch query for cuda is a no-op * [Test][Relay][Pass] Add test case for lambda lift (apache#4317) * [Relay][Frontend][ONNX] operator support: DepthToSpace, SpaceToDepth (apache#4271) * imp module is deprecated (apache#4275) * [VTA] Bug fix for padded load with large inputs (apache#4293) * bug fix for padded load with large inputs * Update TensorLoad.scala * Update test_vta_insn.py * fix inconsistent tag name (apache#4134) * [CodeGen] Add build config option disable_assert to control whether to generate assert (apache#4340) * Bump up CUDA log version in tophub.py (apache#4347) * Add check to ensure input file was successfully opened in NNVM deploy code demo (apache#4315) * [COMMUNITY] Add DISCLAIMER, KEYS for ASF release (apache#4345) * [COMMUNITY] Add DISCLAIMER, KEYS for ASF release * Add file name spec * [Relay][VM][Interpreter] Enable first-class constructors in VM and interpreter via eta expansion (apache#4218) * Fix constructor pretty printing * Make Module::HasDef name consistent with API * Add VM constructor compilation via eta expansion * Lint * Fix CI * Fix failing test * Address comment * Retrigger CI * Retrigger CI * Update dmlc_tvm_commit_id.txt
The tests use fake quant approach so until the tf session, tensors remain in float32.
The test data has to be passed in uint8 because of how the tflite/tvm comparison works.
Abs tolerance up to 1 is allowed for the qnn results. For now input_stats are hardcoded
assuming the tests for the other qnn ops will pass the input data in the same range.