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.

Sign up for GitHub

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

Jump to bottom

Reenable scale unification #2199

Closed
wants to merge 21 commits into from
Closed

Reenable scale unification #2199

wants to merge 21 commits into from

Conversation

vshampor
Copy link
Contributor

@vshampor vshampor commented Oct 13, 2023 •
edited
Loading

Changes

Restored the original, pre-#1778 logic of scale unification and added missing tests for the logic. Added BatchNorm as a quantizable operation (asymmetric, per-channel) to the CPU HW config to handle cases like densenet where batch norm is the first operation in a branch.

Reason for changes

Scales are currently not correctly unified in cases such as #2195.

Related tickets

N/A

Tests

tests/common/quantization/test_quantizer_setup.py
tests/**/quantization/test_unified_scales.py

Fixes: #2195

@github-actions github-actions bot added NNCF PT Pull requests that updates NNCF PyTorch NNCF Common Pull request that updates NNCF Common labels Oct 13, 2023
@vshampor vshampor changed the title Reenable scale unification [DRAFT] Reenable scale unification Oct 17, 2023
@github-actions github-actions bot added NNCF TF Pull requests that updates NNCF TensorFlow NNCF OpenVINO Pull requests that updates NNCF OpenVINO NNCF ONNX Pull requests that updates NNCF ONNX NNCF PTQ Pull requests that updates NNCF PTQ labels Oct 17, 2023
@vshampor vshampor marked this pull request as ready for review October 17, 2023 15:47
@vshampor vshampor requested a review from a team as a code owner October 17, 2023 15:47
@vshampor
Copy link
Contributor Author

I put this out of the draft state to trigger the CI runs first.

@codecov
Copy link

codecov bot commented Oct 20, 2023 •
edited
Loading

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (6539272) 90.82% compared to head (2ef4513) 84.56%.
Report is 6 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2199      +/-   ##
===========================================
- Coverage    90.82%   84.56%   -6.27%     
===========================================
  Files          498      498              
  Lines        45485    45482       -3     
===========================================
- Hits         41314    38464    -2850     
- Misses        4171     7018    +2847
Files Coverage Δ
nncf/common/hardware/opset.py 100.00% <100.00%> (ø)
...ommon/quantization/quantizer_propagation/solver.py 93.89% <ø> (+0.12%) ⬆️
nncf/onnx/graph/metatypes/onnx_metatypes.py 99.58% <100.00%> (+<0.01%) ⬆️
...ncf/openvino/graph/metatypes/openvino_metatypes.py 90.71% <100.00%> (-8.72%) ⬇️
nncf/quantization/algorithms/min_max/algorithm.py 94.84% <ø> (-2.35%) ⬇️
nncf/quantization/algorithms/min_max/backend.py 100.00% <ø> (ø)
...cf/quantization/algorithms/min_max/onnx_backend.py 94.87% <ø> (-0.10%) ⬇️
...uantization/algorithms/min_max/openvino_backend.py 0.00% <ø> (-96.43%) ⬇️
...f/quantization/algorithms/min_max/torch_backend.py 97.38% <ø> (-0.05%) ⬇️
nncf/tensorflow/graph/metatypes/keras_layers.py 96.74% <100.00%> (+<0.01%) ⬆️
... and 5 more

... and 54 files with indirect coverage changes

Flag Coverage Δ
COMMON 43.26% <80.95%> (+1.03%) ⬆️
ONNX 34.72% <56.00%> (-0.02%) ⬇️
OPENVINO ∅ <ø> (∅)
TENSORFLOW 29.67% <56.00%> (-0.01%) ⬇️
TORCH 65.96% <68.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
common 92.74% <85.71%> (-0.59%) ⬇️
torch 93.41% <100.00%> (+<0.01%) ⬆️
tensorflow 94.05% <100.00%> (+0.08%) ⬆️
onnx 93.04% <100.00%> (-0.01%) ⬇️
openvino 25.65% <100.00%> (-68.42%) ⬇️
ptq 67.18% <ø> (-20.48%) ⬇️

@vshampor vshampor force-pushed the fix_concat_grouping branch 2 times, most recently from 4d689b6 to 3b73a97 Compare October 31, 2023 12:06
@vshampor
Copy link
Contributor Author

vshampor commented Nov 8, 2023 •
edited
Loading

ONNX E2E 489 shows accuracy improvement for densenet12 (+0.77% acc.) and resnet50-v2-7 (+0.1% acc) and no difference for other models.

TF E2E 490 shows no significant changes to the regular nightly run.

PTQ build 240 shows accuracy degradation of 0.1% on timm/dpn68 and of 0.2% timm/visformer_small - both are for torch backend only, and for both the INT8 metric for the model is still within 99% FP32.

@vshampor vshampor changed the title [DRAFT] Reenable scale unification Reenable scale unification Nov 17, 2023
@vshampor vshampor force-pushed the fix_concat_grouping branch 2 times, most recently from 519ba21 to caf4000 Compare November 23, 2023 08:46
@github-actions github-actions bot removed NNCF TF Pull requests that updates NNCF TensorFlow NNCF OpenVINO Pull requests that updates NNCF OpenVINO NNCF ONNX Pull requests that updates NNCF ONNX NNCF PTQ Pull requests that updates NNCF PTQ labels Dec 27, 2023
@github-actions github-actions bot added the NNCF ONNX Pull requests that updates NNCF ONNX label Dec 28, 2023
@github-actions github-actions bot added NNCF TF Pull requests that updates NNCF TensorFlow NNCF OpenVINO Pull requests that updates NNCF OpenVINO NNCF PTQ Pull requests that updates NNCF PTQ labels Dec 29, 2023
@vshampor vshampor requested a review from KodiaqQ January 2, 2024 13:32
@vshampor vshampor force-pushed the fix_concat_grouping branch 2 times, most recently from 7bc7988 to ddd930c Compare February 7, 2024 10:52
@vshampor
Copy link
Contributor Author

@KodiaqQ PTQ build 285 shows a single regression vs build 286 on timm/visformer_small. Overall metric is still within 99% FP32.

@KodiaqQ
Copy link
Collaborator

KodiaqQ commented Feb 16, 2024

@KodiaqQ PTQ build 285 shows a single regression vs build 286 on timm/visformer_small. Overall metric is still within 99% FP32.

If the PTQ build is red, then we need to update the reference for the timm/visformer_small model as well. We can do it in this PR or follow-up, doesn't matter. But we should do it to keep builds green.
Also, is there any observation of why this model shows lower accuracy than the OV and ONNX versions?

@KodiaqQ KodiaqQ self-requested a review February 16, 2024 11:01
@vshampor vshampor closed this Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KodiaqQ KodiaqQ Awaiting requested review from KodiaqQ

Assignees

@KodiaqQ KodiaqQ

Labels
NNCF Common Pull request that updates NNCF Common NNCF ONNX Pull requests that updates NNCF ONNX NNCF OpenVINO Pull requests that updates NNCF OpenVINO NNCF PT Pull requests that updates NNCF PyTorch NNCF PTQ Pull requests that updates NNCF PTQ NNCF TF Pull requests that updates NNCF TensorFlow
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Concat scales not being grouped
2 participants
@vshampor @KodiaqQ