Enhance shape inference: ParseData/Transpose/QuantizeLinear #3806
Conversation
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
onnx/defs/tensor/defs.cc
Outdated
| } else { | ||
| // check if any perm is repeated | ||
| if (seen[fromDimIndex]) { | ||
| fail_type_inference("Attribute perm for Transpose cannot be repeated: ", fromDimIndex); |
There was a problem hiding this comment.
nit: Attribute perm for Transpose has repeated entry for value fromIndex? or something similar?
onnx/defs/tensor_proto_util.cc
Outdated
| @@ -64,6 +64,10 @@ namespace ONNX_NAMESPACE { | |||
| res.insert(res.end(), data.begin(), data.end()); \ | |||
| return res; \ | |||
| } \ | |||
| if (tensor_proto->data_type() == TensorProto_DataType_STRING) { \ | |||
| fail_shape_inference("The data_type of tensor: ", tensor_proto->name(), \ | |||
| " cannot be string because it has raw_data which disallows string."); \ | |||
There was a problem hiding this comment.
nit: tensor_proto->name() data type is string. string content is required to be stored in repeated bytes string_data field. raw_data type cannot be string.
There was a problem hiding this comment.
Both updated for improving error messages. Thanks
| @@ -55,14 +55,14 @@ ONNX_OPERATOR_SET_SCHEMA( | |||
| .SetDoc(QuantizeLinear_ver13_doc) | |||
| .TypeAndShapeInferenceFunction( | |||
| [](ONNX_NAMESPACE::InferenceContext& ctx) { | |||
| if (ctx.getNumInputs() == 3) { | |||
| if (ctx.getNumInputs() == 3 && ctx.getInputType(2) != nullptr) { | |||
There was a problem hiding this comment.
this looks good. Wondering how can we avoid this issue in case of all optional inputs. Right now I think only QuantizeLinear does type propagation from optional input to output... but if tomorrow there is some other op which does the same then the type inference will have to take care of this condition. May not be worth it right now... @gramalingam thoughts?
There was a problem hiding this comment.
Yes I was thinking of it too... This situation can happen again in the future. Perhaps we can extend existing getNumInputs with more parameter to represent whether it disallows optional input for certain index (or a set of indexes)
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
|
The change looks good... please add tests |
Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
Good point. I added 2 tests for checking repeated perm for Transpose and empty string for QuantizeLinear. Regarding to test for ParseData with raw_data and string type, actually ParseData does not support parse string type now so the condition cannot be tested... If that is the case, do we still need to add the check here? |
Description
2 Transpose should check repeated perm to prevent issues
3 If third input (y_zero_point) of QuantizeLinear is an empty string, shape inference should still work because y_zero_point is an optional input
Motivation and Context