ZhennanQin commented on issue #13697: [MKLDNN] Enable signed int8 support for convolution. URL: https://github.com/apache/incubator-mxnet/pull/13697#issuecomment-452135740 @KellenSunderland Really thanks for your engaged reviewing. @szha Thanks for your good suggestion. There're 2 APIs breaking need to solve, I want to clarify them here one by one. For include/mxnet/c_api.h: ``` MXNET_DLL int MXQuantizeSymbol(SymbolHandle sym_handle, SymbolHandle *ret_sym_handle, const mx_uint num_excluded_symbols, const char **excluded_symbols, const mx_uint num_offline, const char **offline_params, const char *quantized_dtype, const bool calib_quantize); const char *quantized_dtype); ``` This PR wants to remove parameter `calib_quantize`. As this API is a part of experimental feature `quantization`, which is unstable and may continuously change in future, I suggest to change it in this PR to make this API more stable and clear, because we have to clean it before feature stable anyway. But if you think it's OK to keep it as a dummy parameter here, I'm fine. For include/mxnet/ndarray.h: ``` explicit NDArray(const mkldnn::memory *mkldnn_mem, bool static_data = true); ``` This PR wants to replace it with a new designed API, because shared_ptr is better for memory management. As I said, it's not designed as a user API, but an internal API for backend implementation. If you think it's worth to make new API along with old API, I think it's OK for me. The only weird thing is, the old API won't be used anymore in MXNet. Can you give your final decision for each of them? Then I will refactor this PR. Thanks again.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
