Skip to content

Fix resize shape inference issue in opset10#2294

Merged
ebarsoum merged 7 commits intoonnx:masterfrom
BowenBao:resize_opset10_shape_inference
Sep 15, 2019
Merged

Fix resize shape inference issue in opset10#2294
ebarsoum merged 7 commits intoonnx:masterfrom
BowenBao:resize_opset10_shape_inference

Conversation

@BowenBao
Copy link
Copy Markdown
Contributor

The issue is found in microsoft/onnxruntime#1756, that the updated opset 10 resize shape inference in defs/tensor/old.cc is failing Yolov3 https://github.com/onnx/models/tree/master/vision/object_detection_segmentation/yolov3.

This is a simple fix to bring back and use the version of shape inference functions before opset11.

@daquexian @hariharans29 @yuslepukhin

@BowenBao BowenBao requested a review from a team as a code owner September 10, 2019 18:10
Comment thread onnx/defs/tensor/old.cc Outdated
Comment thread onnx/defs/tensor/utils.cc
Comment thread onnx/defs/tensor/utils.cc Outdated
@prasanthpul prasanthpul added this to the 1.6 milestone Sep 10, 2019
@BowenBao BowenBao force-pushed the resize_opset10_shape_inference branch from d720827 to 5d5853a Compare September 11, 2019 20:20
Comment thread onnx/defs/tensor/utils.cc
int64_t dim_value = static_cast<int64_t>(std::floor(
static_cast<float>(input_shape.dim(i).dim_value()) * scales_data[i]));
// If output_shape has dim_value, we validate the caculated result
// If output_shape doesn's have one, we set it to the scaled result
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The utility function unifyDim (see

inline void unifyDim(Dim& dim, int64_t value) {
) does this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm only reverting back a change that causes regression in resize shape inference. Thus I want to avoid modifying the original code as much as possible ...

Comment thread onnx/defs/tensor/utils.cc
auto* output_shape = getOutputShape(ctx, 0);
const auto scales = ctx.getInputData(1);

if (output_shape->dim_size() > 0) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These checks are fine, but I believe that they are not needed. The way inference works, these functions return an output type and shape, and the caller will check for compatibility with existing type/shape and combine them appropriately.

@wschin
Copy link
Copy Markdown
Collaborator

wschin commented Sep 12, 2019

Is it feasible to add a test to prevent this problem?

@gramalingam
Copy link
Copy Markdown
Contributor

Some description of what went wrong would also be helpful (if we know) along with the testcase.

@BowenBao
Copy link
Copy Markdown
Contributor Author

Is it feasible to add a test to prevent this problem?

That's a good point. For shape inference tests we rarely test with specified opset version. That implies by default whenever an operator gets updated, their previous versions are no longer tested.

@ebarsoum ebarsoum merged commit 57b5193 into onnx:master Sep 15, 2019
jcwchen pushed a commit to jcwchen/onnx that referenced this pull request Sep 23, 2020
* fix resize shape inference issue in opset10

* include opset10 upsample as well

* nit: const auto*

* rename 'opset7' to 'opset7_to_10'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants