[GitHub] [incubator-mxnet] mozga-intel commented on a change in pull request #20514: [REFACTOR] Asymmetric Quantization: deduplicate methods

[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