Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[REFACTOR] Asymmetric Quantization: deduplicate methods #20514

Merged
merged 6 commits into from
Sep 2, 2021
Merged

[REFACTOR] Asymmetric Quantization: deduplicate methods #20514

merged 6 commits into from
Sep 2, 2021

Conversation

sfraczek
Copy link
Contributor

@sfraczek sfraczek commented Aug 11, 2021 •
edited
Loading

Description

Just some refactoring of asymmetric quantization.

Checklist

Essentials

  • PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • Code is well-documented

Changes

[x] Mainly deduplication of methods,
[x] Secondly unify naming according to v1.x (onednn->mkldnn),
[x] Also clang-formatting.

Comments

@sfraczek sfraczek requested a review from szha as a code owner August 11, 2021 15:58
@mxnet-bot
Copy link

Hey @sfraczek , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [centos-gpu, clang, unix-cpu, unix-gpu, windows-gpu, website, sanity, centos-cpu, edge, miscellaneous, windows-cpu]

Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

@mseth10 mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress labels Aug 11, 2021
@mseth10 mseth10 added pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Aug 12, 2021
@sfraczek
Copy link
Contributor Author

@mxnet-bot run ci [unix-cpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [unix-cpu]

@mseth10 mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Aug 12, 2021
@mozga-intel
Copy link
Contributor

@sfraczek, Thanks! Could you please expand a little bit and explain in more detail what this pull request takes in?

@sfraczek
Copy link
Contributor Author

renaming variables
replacing QuantizeFcShiftedQuantization and FcFcShiftedQuantization with single function because they are identical.

mozga-intel
mozga-intel reviewed Aug 13, 2021
View reviewed changes
src/operator/quantization/asymmetric_quantize_graph_pass.cc Outdated
@@ -254,21 +215,21 @@ static Graph OneDNNShiftedQuantization(Graph&& g) {
}
});
if (quantize_fc_counter > 0) {
LOG(INFO) << "applied shifted quantization on QUANTIZE->FC " << quantize_fc_counter
LOG(INFO) << "applied asymmetric quantization on QUANTIZE->FC " << quantize_fc_counter
Copy link
Contributor

@mozga-intel mozga-intel Aug 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Please have a look at it, here we start with a lowercase letter, but #185 starts with an uppercase letter. What do you think of keeping the same rules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks I will make it uppercase too.

src/operator/quantization/quantize_v2-inl.h
@@ -41,7 +41,7 @@ struct QuantizeV2Param : public dmlc::Parameter<QuantizeV2Param> {
int out_type;
dmlc::optional<float> min_calib_range;
dmlc::optional<float> max_calib_range;
dmlc::optional<bool> shifted;
dmlc::optional<bool> shifted_output;
Copy link
Contributor

@mozga-intel mozga-intel Aug 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The dmlc::optional is found a use for representing a value that may or may not be present. The std::optional was introduced in C++17, so it means that we can try to replace this one by a new one. Unfortunately, there is a bunch of branches have not supported C++17 yet. An exceptation is the master branch that supports c++17, well, maybe is worth making an internal wrapper. On second thoughts, it could be looks like: [proposition]

#define MXNET_HAS_OPTIONAL() 0
#if __cplusplus >= 201703L
#  ifdef __has_include
#    if __has_include(<optional>)
#      include <optional>
#      undef MXNET_HAS_OPTIONAL
#      define MXNET_HAS_OPTIONAL() 1
#    endif
#  endif
#endif

#if MXNET_HAS_OPTIONAL()
#warning "optional"
// Please use std::optional<T>
#else
#warning "no optional"
// Please use dmlc::optional<T>
#endif

What are the advantages of using it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think v1.x is more strongly backward compatibility oriented. I think it would be better to keep such change only to master. dmlc::optional is used in a lot of places as well as other functions so it might be hard to justify rewriting parts of it. I don't know what are advantages of either implementations.
Also, in this PR I would prefer to keep consistency with what is already used in operators.

tests/python/quantization/test_quantization.py Outdated
@@ -1255,7 +1255,7 @@ def get_threshold(nd):


@with_seed()
def test_onednn_shifted_quantize_fc():
def test_mkldnn_shifted_quantize_fc():
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it might not be better to re-name this function on the off-chance? ~> test_mkldnn_asymetric_quantize_fc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@mseth10 mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-awaiting-review PR is waiting for code review labels Aug 13, 2021
@mseth10 mseth10 added pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Aug 13, 2021
@sfraczek
Copy link
Contributor Author

@mxnet-bot run ci [unix-cpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [unix-cpu]

@mseth10 mseth10 removed the pr-work-in-progress PR is still work in progress label Aug 30, 2021
@mseth10 mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review and removed pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review labels Aug 30, 2021
@mseth10 mseth10 added pr-awaiting-review PR is waiting for code review and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Aug 31, 2021
@akarbown akarbown merged commit 42a48a7 into apache:v1.x Sep 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mozga-intel mozga-intel mozga-intel approved these changes

@szha szha Awaiting requested review from szha szha is a code owner

Assignees
No one assigned
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sfraczek @mxnet-bot @mozga-intel @mseth10 @akarbown