You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2021/08/11 15:58:22 UTC
[GitHub] [incubator-mxnet] sfraczek opened a new pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
sfraczek opened a new pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514
## Description ##
Just some refactoring of asymmetric quantization.
## Checklist ##
### Essentials ###
- [x] PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
- [x] Changes are complete (i.e. I finished coding on this PR)
- [x] 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 ##
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mozga-intel commented on a change in pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
Posted by GitBox <gi...@apache.org>.
mozga-intel commented on a change in pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514#discussion_r688373879
##########
File path: 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;
Review comment:
The dmlc::optional<T> 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 like this: [proposition]
```cpp
#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?
##########
File path: src/operator/quantization/asymmetric_quantize_graph_pass.cc
##########
@@ -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
Review comment:
Please have a look at: here we start with a lower case, but #185 starts with an upper case? If it's a refactor, maybe is worth changing it
##########
File path: tests/python/quantization/test_quantization.py
##########
@@ -1255,7 +1255,7 @@ def get_threshold(nd):
@with_seed()
-def test_onednn_shifted_quantize_fc():
+def test_mkldnn_shifted_quantize_fc():
Review comment:
I wonder if it might not be better to re-name this function on the off-chance? ~> test_mkldnn_asymetric_quantize_fc?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] sfraczek commented on pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
Posted by GitBox <gi...@apache.org>.
sfraczek commented on pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514#issuecomment-898271815
renaming variables
replacing QuantizeFcShiftedQuantization and FcFcShiftedQuantization with single function because they are identical.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mozga-intel commented on a change in pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
Posted by GitBox <gi...@apache.org>.
mozga-intel commented on a change in pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514#discussion_r688338015
##########
File path: src/operator/quantization/asymmetric_quantize_graph_pass.cc
##########
@@ -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
Review comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mozga-intel commented on a change in pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
Posted by GitBox <gi...@apache.org>.
mozga-intel commented on a change in pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514#discussion_r688373879
##########
File path: 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;
Review comment:
The dmlc::optional<T> 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]
```cpp
#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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] sfraczek commented on a change in pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
Posted by GitBox <gi...@apache.org>.
sfraczek commented on a change in pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514#discussion_r688402873
##########
File path: src/operator/quantization/asymmetric_quantize_graph_pass.cc
##########
@@ -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
Review comment:
thanks I will make it uppercase too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] akarbown merged pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
Posted by GitBox <gi...@apache.org>.
akarbown merged pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514#issuecomment-896947848
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mozga-intel commented on pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
Posted by GitBox <gi...@apache.org>.
mozga-intel commented on pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514#issuecomment-898261785
@sfraczek, Thanks! Could you please expand a little bit and explain in more detail what this pull request takes in?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mozga-intel commented on a change in pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
Posted by GitBox <gi...@apache.org>.
mozga-intel commented on a change in pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514#discussion_r688373879
##########
File path: 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;
Review comment:
The dmlc::optional<T> 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 the second thoughts it could be looks like: [proposition]
```cpp
#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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514#issuecomment-908384780
Jenkins CI successfully triggered : [unix-cpu]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] sfraczek commented on pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
Posted by GitBox <gi...@apache.org>.
sfraczek commented on pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514#issuecomment-897703480
@mxnet-bot run ci [unix-cpu]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514#issuecomment-897703568
Jenkins CI successfully triggered : [unix-cpu]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] sfraczek commented on pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
Posted by GitBox <gi...@apache.org>.
sfraczek commented on pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514#issuecomment-908384684
@mxnet-bot run ci [unix-cpu]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] sfraczek commented on a change in pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
Posted by GitBox <gi...@apache.org>.
sfraczek commented on a change in pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514#discussion_r688497682
##########
File path: tests/python/quantization/test_quantization.py
##########
@@ -1255,7 +1255,7 @@ def get_threshold(nd):
@with_seed()
-def test_onednn_shifted_quantize_fc():
+def test_mkldnn_shifted_quantize_fc():
Review comment:
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] sfraczek commented on pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
Posted by GitBox <gi...@apache.org>.
sfraczek commented on pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514#issuecomment-908384684
@mxnet-bot run ci [unix-cpu]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] sfraczek commented on a change in pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
Posted by GitBox <gi...@apache.org>.
sfraczek commented on a change in pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514#discussion_r688415411
##########
File path: 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;
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #20514:
URL: https://github.com/apache/incubator-mxnet/pull/20514#issuecomment-908384780
Jenkins CI successfully triggered : [unix-cpu]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org