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

[MinMax] Move get_statistic_collector from backend to common #3132

Merged

Conversation

rk119
Copy link
Contributor

@rk119 rk119 commented Dec 6, 2024

Changes

  • Added a common REDUCERS_MAP in nncf/experimental/common/tensor_statistics/collectors.py for backends PT, FX and ONNX.
  • Removed redundant reducers maps in backend specific collectors and updated relevant tests that were previously using them.
  • Added the get_statistic_collector method in the common nncf/quantization/algorithms/min_max/algorithm.py file and defined a reducers_map property to each backend file.
  • Changed the default value of inplace_statistics to False in nncf/quantization/advanced_parameters.py since the attribute inplace in get_statistic_collector in nncf/quantization/algorithms/min_max/algorithm.py is now passed for all backends, it needs to be False by default and True only for OpenVino.
  • Removed the get_statistic_collector method definition from PT, FX, ONNX and OpenVino min_max backend specific files.
  • Removed the abstract get_statistic_collector method from min_max/backend.py as it is no longer needed.

Reason for changes

To remove redundant method definitions in all backends.

Related tickets

#3120

@github-actions github-actions bot added NNCF PT Pull requests that updates NNCF PyTorch experimental NNCF ONNX Pull requests that updates NNCF ONNX NNCF PTQ Pull requests that updates NNCF PTQ labels Dec 6, 2024
@rk119 rk119 changed the title [NNCF][MinMax] Move get_statistic_collector from backend to common [WIP][MinMax] Move get_statistic_collector from backend to common Dec 6, 2024
@rk119 rk119 marked this pull request as ready for review December 9, 2024 07:52
@rk119 rk119 requested a review from a team as a code owner December 9, 2024 07:52
@rk119
Copy link
Contributor Author

rk119 commented Dec 9, 2024

Hi @daniil-lyakhov,

Can you run the checks and review the changes made so far in this PR? The min_max tests do not require any updates so far from what I have observed.

@rk119 rk119 changed the title [WIP][MinMax] Move get_statistic_collector from backend to common [MinMax] Move get_statistic_collector from backend to common Dec 9, 2024
@daniil-lyakhov daniil-lyakhov self-assigned this Dec 9, 2024
@github-actions github-actions bot added the NNCF OpenVINO Pull requests that updates NNCF OpenVINO label Dec 9, 2024
Copy link
Collaborator

@daniil-lyakhov daniil-lyakhov left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! Couple comments from my side:

nncf/quantization/advanced_parameters.py Outdated Show resolved Hide resolved
nncf/quantization/algorithms/min_max/algorithm.py Outdated Show resolved Hide resolved
rk119 and others added 2 commits December 11, 2024 21:24
Co-authored-by: Daniil Lyakhov <[email protected]>
Co-authored-by: Daniil Lyakhov <[email protected]>
@daniil-lyakhov daniil-lyakhov self-requested a review December 11, 2024 17:26
rk119 and others added 2 commits December 12, 2024 14:07
Co-authored-by: Daniil Lyakhov <[email protected]>
Copy link
Collaborator

@daniil-lyakhov daniil-lyakhov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@daniil-lyakhov
Copy link
Collaborator

@AlexanderDokuchaev, please review and merge if you have no comments

@AlexanderDokuchaev
Copy link
Collaborator

@rk119 Well done

@AlexanderDokuchaev AlexanderDokuchaev merged commit 36a8420 into openvinotoolkit:develop Dec 12, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants