Fix Squeeze shape inference #3516
Conversation
Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>
gramalingam
added
operator
|
Can't we move line 1634 to after the first loop to fix (2)? |
onnx/defs/tensor/defs.cc
Outdated
| for (int i = 0; i < input_ndim; ++i) { | ||
| if (!input_shape.dim(i).has_dim_value()) { | ||
| return; | ||
| if(!input_shape.dim(i).has_dim_value() && (axes_not_specified || std::find(axes.begin(), axes.end(), i) != axes.end())) { |
There was a problem hiding this comment.
In principle, I think we can omit the || std::find(axes.begin(), axes.end(), i) != axes.end()) part. Thus, we assume that the axes specification is correct for shape-inference purpose. If it is incorrect, the runtime execution needs to catch and report it anyway. Does that make sense? However, this would require minor changes to the loop below in line 1657 ... we should check if input_shape_has_dim_value before accessing it.
There was a problem hiding this comment.
And what should happen in the loop below if has_dim_value is false? I still see no option other than giving up the shape inference.
I understand your point about the checking that the symbolic size is actually 1 at runtime, that should happen anyway. But still shape inference should give up (i.e. restrain from specifying anything about the output shape) in this case.
There was a problem hiding this comment.
This is a fine point of distinction. In the loop below, if the outer condition is true (that is, axes is specified and contains i), then we should omit dim(i) from the output. The idea is that the shape-inference contract is that it indicates what the output shape will be IF the op executes successfully and produces an output. Type and shape inference does NOT guarantee that the op will execute successfully (because of the need for some runtime checks). Does that make sense?
There was a problem hiding this comment.
If you are not convinced, we can merge this as is, and we can do this as a separate PR later after discussion.
There was a problem hiding this comment.
The idea is that the shape-inference contract is that it indicates what the output shape will be IF the op executes successfully and produces an output. Type and shape inference does NOT guarantee that the op will execute successfully (because of the need for some runtime checks). Does that make sense?
Ok so you are suggesting to omit dim(i) from the output if:
- dim(i) is numerical and equal to 1 (usual case) OR
- dim(i) is symbolic
In the second case, runtimes should obviously check that dim(i) is actually 1 when the Squeeze op is evaluated. Is that correct?
There was a problem hiding this comment.
I have added the change, please let me know if it's ok
Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>
Yes, that worked |
|
@gramalingam any news on this? |
Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com>
* Return early only if axes spec wants to act on symbolic dim Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com> * Dont' predict a rank of 0 when shape can't be inferred Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com> * Remove symbolic dims in the axes spec, don't give up inference Signed-off-by: Matteo Salvarezza <matteo.salvarezza@gmail.com> Signed-off-by: hwangdeyu <dejack953@outlook.com>
Fixes #3515
There seem to be 2 problems here:
I have fixed (1) but (2) still remains to be addressed.