fix type inference issue (scalar initializers and Resize) #2983
Conversation
| for (int i = 0 ; i < tp.dims_size(); ++i) { | ||
| shape->add_dim()->set_dim_value(tp.dims(i)); | ||
| } | ||
| TensorShapeProto* shape = initializerTensorType->mutable_shape(); |
There was a problem hiding this comment.
I think this check -> tp.dims_size() !=0 is needed before output tensor shape is initialized?
This can cause issues because has_shape for this tensor will return true from here onwards and depending what the calling code (whether it does necessary checks or not ) it can cause segfaults
See PR : #1951
There was a problem hiding this comment.
The dims_size check here is not related to whether the shape is known or not known. Here, the shape is known exactly. If dims_size is 0, it means it is rank 0 (a scalar). This check is different from a "has_shape" kind of check.
There was a problem hiding this comment.
I added tp.dims_size() != 0 because 3 tests in shape_inference_test will fail without this. My thought was same as @askhade that this can check whether the shape is initialized.
If dims_size=0 means a scalar, there might be other potential bugs in splitToSequence because after applying this PR, the ranks of initializer and input are different: RuntimeError: Inferred shape and existing shape differ in rank: (0) vs (1) .
There was a problem hiding this comment.
It is possible that there are bugs elsewhere (in requiring rank 0 or rank 1 for supposedly-scalar inputs). Or, if the spec calls for rank 1, and the model uses rank 0, it is an error in the model. Adding the check here will only mask problems elsewhere, I think.
There was a problem hiding this comment.
The split_to_sequence testcases had an issue. I fixed them. Hope this goes through the CI.
|
The fix of resize, initializer and tests looks good to me. Thank you for proposing this @gramalingam |
* fix type inference issue * fix initializer shape in split_to_sequence test-cases Signed-off-by: daquexian <daquexian566@gmail.com>
* fix type inference issue * fix initializer shape in split_to_sequence test-cases Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com>
* fix type inference issue * fix initializer shape in split_to_sequence test-cases
* fix type inference issue * fix initializer shape in split_to_sequence test-cases
* fix type inference issue * fix initializer shape in split_to_sequence test-cases
Fix two issues in type inference
(a) Scalar initializers type (tensor element type) was not being set.
(b) Resize type inference had the check for shape-inference before the type-inference (failing to propagate type).
These changes fix the issue mentioned in #2965