Skip to content
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

Merged
merged 2 commits into from
Nov 12, 2019
Merged

Add test for the qnn_add operator #4282

merged 2 commits into from
Nov 12, 2019

Conversation

inadob
Copy link
Contributor

@inadob inadob commented Nov 8, 2019

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.

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.
@inadob
Copy link
Contributor Author

inadob commented Nov 8, 2019

@FrozenGene @anijain2305 can you please review this patch

@FrozenGene
Copy link
Member

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

inadob added a commit to inadob/tvm that referenced this pull request Nov 8, 2019
A test for qnn_mul has to be added when the qnn elemwise tests (apache#4282) get merged.
@anijain2305
Copy link
Contributor

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.

Copy link
Contributor

@anijain2305 anijain2305 left a 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 :)

tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
@FrozenGene
Copy link
Member

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.

@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
Copy link
Contributor

@anijain2305 anijain2305 left a 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?

Copy link
Member

@zhiics zhiics left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zhiics zhiics merged commit e680611 into apache:master Nov 12, 2019
@zhiics
Copy link
Member

zhiics commented Nov 12, 2019

Thanks @inadob @anijain2305 @FrozenGene

zxy844288792 pushed a commit to neo-ai/tvm that referenced this pull request Nov 13, 2019
* 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
tqchen pushed a commit that referenced this pull request Nov 15, 2019
A test for qnn_mul has to be added when the qnn elemwise tests (#4282) get merged.
zxy844288792 pushed a commit to zxy844288792/tvm that referenced this pull request Nov 15, 2019
A test for qnn_mul has to be added when the qnn elemwise tests (apache#4282) get merged.
zxy844288792 pushed a commit to zxy844288792/tvm that referenced this pull request Nov 15, 2019
A test for qnn_mul has to be added when the qnn elemwise tests (apache#4282) get merged.
kevinthesun pushed a commit to neo-ai/tvm that referenced this pull request Nov 25, 2019
* [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants