Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix resize shape inference issue in opset10 #2294

Merged
merged 7 commits into from
Sep 15, 2019

Conversation

BowenBao
Copy link
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
onnx/defs/tensor/old.cc Outdated Show resolved Hide resolved
@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
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
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
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 ...

auto* output_shape = getOutputShape(ctx, 0);
const auto scales = ctx.getInputData(1);

if (output_shape->dim_size() > 0) {
Copy link
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
Contributor

wschin commented Sep 12, 2019

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

@gramalingam
Copy link
Contributor

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

@BowenBao
Copy link
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.

None yet

6 participants