Skip to content

Comments

dnn cleanup: On-fly-quantization removal#24980

Merged
asmorkalov merged 3 commits intoopencv:5.xfrom
fengyuentau:on-fly-quantization-removal
Feb 16, 2024
Merged

dnn cleanup: On-fly-quantization removal#24980
asmorkalov merged 3 commits intoopencv:5.xfrom
fengyuentau:on-fly-quantization-removal

Conversation

@fengyuentau
Copy link
Member

@fengyuentau fengyuentau commented Feb 8, 2024

On-fly-quantization is first introduced via #20228. We decided to remove it but keep int8 layers implementation because on-fly-quantization is less practical given the fact that there has been so many dedicated tools for model quantization.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@fengyuentau fengyuentau added category: dnn cleanup Code cleanup (e.g, drop legacy C-API, legacy unmaintained code) labels Feb 8, 2024
@fengyuentau fengyuentau added this to the 5.0 milestone Feb 8, 2024
@fengyuentau fengyuentau self-assigned this Feb 8, 2024
@fengyuentau
Copy link
Member Author

Should we keep netwasQuantized? It is used only in two ways:

  1. Check whether backend supports quantized model;
  2. Calculate blob size in getMemoryConsumption.

@fengyuentau
Copy link
Member Author

Ubuntu2004-x64-CUDA / BuildAndTest:

[ RUN      ] DNNTestNetwork.FastNeuralStyle_eccv16/0, where GetParam() = CUDA/CUDA
/home/ci/opencv/modules/dnn/test/test_common.impl.hpp:75: Failure
Expected: (normL1) <= (l1), actual: 0.000708958 vs 0.0007
First run  |ref| = 348.99359130859375

I took a loot at the model and it has nothing to do with quantization, so this should not be related to the changes in this PR.

@vpisarev
Copy link
Contributor

vpisarev commented Feb 8, 2024

@fengyuentau, since the error is still very low, probably it makes sense just slightly increase the tolerance threshold, e.g. from 0.0007 to 0.001

@fengyuentau
Copy link
Member Author

@fengyuentau, since the error is still very low, probably it makes sense just slightly increase the tolerance threshold, e.g. from 0.0007 to 0.001

Yes, sure. I want to find out whether it is sporadic and do it in antoher PR.

@asmorkalov
Copy link
Contributor

Related: #24993

Comment on lines -626 to -632
CV_WRAP void getInputDetails(CV_OUT std::vector<float>& scales, CV_OUT std::vector<int>& zeropoints) const;

/** @brief Returns output scale and zeropoint for a quantized Net.
* @param scales output parameter for returning output scales.
* @param zeropoints output parameter for returning output zeropoints.
*/
CV_WRAP void getOutputDetails(CV_OUT std::vector<float>& scales, CV_OUT std::vector<int>& zeropoints) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose that getInputDetails and getOutputDetails should be presumed. We support pre-quantized networks and the mentioned API is not related to on-fly quantization.

Copy link
Member Author

Choose a reason for hiding this comment

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

They are only used in test_int8_layers.cpp which quantizes nets on the fly (thus you need to quantize the inputs manually with the inputDetails). I don't think they are useful anymore given the fact that we are removing on-fly quantization.

@asmorkalov asmorkalov requested a review from vpisarev February 14, 2024 06:50
@vpisarev
Copy link
Contributor

@fengyuentau, thank you! We discussed it briefly with @asmorkalov. Indeed, the functionality shall be removed and the tests should be commented off. It's recommended to use // to comment off tests to make the patch easier to read. /* and */, applied to big pieces of code are difficult to analyze in a patch.

@fengyuentau
Copy link
Member Author

I think netWasQuantized should be removed as well since we are going to have type inference and whether a net (quantized or not) should be supported by any backend should be determined by whether all the layers are supported in the backend.

@fengyuentau
Copy link
Member Author

@fengyuentau, thank you! We discussed it briefly with @asmorkalov. Indeed, the functionality shall be removed and the tests should be commented off. It's recommended to use // to comment off tests to make the patch easier to read. /* and */, applied to big pieces of code are difficult to analyze in a patch.

I used #if 0 as sugguested by @opencv-alalek.

Copy link
Contributor

@asmorkalov asmorkalov left a comment

Choose a reason for hiding this comment

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

👍

@asmorkalov asmorkalov merged commit d4fd515 into opencv:5.x Feb 16, 2024
@fengyuentau fengyuentau deleted the on-fly-quantization-removal branch February 17, 2024 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: dnn cleanup Code cleanup (e.g, drop legacy C-API, legacy unmaintained code)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants