Skip to content

Resize op test update to opset 13#3361

Merged
postrational merged 4 commits intoonnx:masterfrom
tomdol:resize_downsample_test_adjustments
Mar 29, 2021
Merged

Resize op test update to opset 13#3361
postrational merged 4 commits intoonnx:masterfrom
tomdol:resize_downsample_test_adjustments

Conversation

@tomdol
Copy link
Copy Markdown
Contributor

@tomdol tomdol commented Mar 25, 2021

The changes in this PR are about one of the Resize op tests: test_resize_downsample_sizes_nearest_tf_half_pixel_for_nn

The current version of this test uses a model with opset_import=11. Given the specification of opset 11 for Resize the model isn't entirely correct. Only one of the last two inputs of Resize should be specified but in the model both are connected to the respective graph inputs. scales input is not optional in opset 11 and the test relies on the values of sizes. The workaround in the current version of the test is that the scales input is connected to a graph input with a zero-dimension. There's also a corresponding input value in the test's directory containing an empty array. A similar trick was applied to the non-optional roi input which is supposed to be ignored in this test because of the coordinate transformation mode.

I propose the following changes to the test:

  • switch the test model to opset 13 because of the updated specification which lets a correct model to be created for this test
  • set both roi and scales inputs in the test model to empty strings because they do not affect this test - this way the op will be given to relevant inputs
  • remove the obsolete input values test_data_set_0/input_1.pb and test_data_set_0/input_1.pb
  • rename test_data_set_0/input_3.pb to test_data_set_0/input_1.pb - this way the test runner will provide 2 input values to an operator with 2 inputs

@tomdol tomdol requested a review from a team as a code owner March 25, 2021 10:11
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 25, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

Update test_resize_downsample_sizes_nearest_tf_half_pixel_for_nn to o…
68643ba 
…pset 13

Signed-off-by: tomdol <tomasz.dolbniak@intel.com>
@postrational postrational requested a review from jcwchen March 25, 2021 18:32
@postrational
Copy link
Copy Markdown
Contributor

postrational commented Mar 25, 2021

@jcwchen could you please verify this updated test using ONNX Runtime?

@jcwchen
Copy link
Copy Markdown
Member

jcwchen commented Mar 25, 2021

@jcwchen could you please verify this updated test using ONNX Runtime?

Yes, they pass ORT backend test and onnx.checker. Thanks.

@postrational postrational added this to the 1.9 milestone Mar 25, 2021
@postrational postrational merged commit d946f12 into onnx:master Mar 29, 2021
@tomdol tomdol mentioned this pull request Mar 31, 2021
Merged
@fdwr
Copy link
Copy Markdown
Contributor

fdwr commented May 4, 2021 •
edited
Loading

@tomdol: We see some failures after updating ONNX in ONNX runtime, and I found it's because some tensors have duplicate names:

test_resize_downsample_sizes_nearest_tf_half_pixel_for_nn\test_data_set_0\

input_0.pb - Tensor "X", datatype: float32, dimensions: 1,1,4,4, (64 bytes): 1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16
input_1.pb - Tensor "sizes", datatype: int64, dimensions: 4, (32 bytes): 1,1,3,2
input_2.pb - Tensor "scales", datatype: float32, dimensions: 0, (0 bytes)
input_3.pb - Tensor "sizes", datatype: int64, dimensions: 4, (32 bytes): 1,1,3,2

Can this be fixed, updating the name in "input_1.pb" to "roi"? I'll just disable that test in meantime. Thanks.

@tomdol
Copy link
Copy Markdown
Contributor Author

tomdol commented May 5, 2021

@fdwr will do, sorry I missed that. I'll let you know when the PR is ready.

@tomdol
Copy link
Copy Markdown
Contributor Author

tomdol commented May 5, 2021

@fdwr I've re-checked the test and it looks correct to me. The tested model expects the "X" and "sizes" inputs to be passed in. "roi" and "scales" are set to empty strings according to the documentation to indicate that they are optional and should be disregarded for this model.

The input data in test_resize_downsample_sizes_nearest_tf_half_pixel_for_nn/test_data_set_0 now contains 2 files instead of 4:

>>> onnx.TensorProto.FromString(open("input_0.pb", "rb").read())
dims: 1
dims: 1
dims: 4
dims: 4
data_type: 1
name: "X"
raw_data: "\000\000\200?\000\000\000@\000\000@@\000\000\200@\000\000\240@\000\000\300@\000\000\340@\000\000\000A\000\000\020A\000\000 A\000\0000A\000\000@A\000\000PA\000\000`A\000\000pA\000\000\200A"

>>> onnx.TensorProto.FromString(open("input_1.pb", "rb").read())
dims: 4
data_type: 7
name: "sizes"
raw_data: "\001\000\000\000\000\000\000\000\001\000\000\000\000\000\000\000\003\000\000\000\000\000\000\000\002\000\000\000\000\000\000\000"

The input names match the operator's input names in the model. Can you please verify on your end?

@tomdol tomdol deleted the resize_downsample_test_adjustments branch May 5, 2021 15:32
@fdwr
Copy link
Copy Markdown
Contributor

fdwr commented May 5, 2021 •
edited
Loading

@tomdol : Sorry for the false alarm 😞. Our test copies the new data, but the copying script left behind the existing old tensors which were deleted in your PR (as this test case was updated inline). The test passes now after wiping the directory 😅. Thanks for your change - it was kinda silly before that the tensor was empty. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@postrational postrational postrational approved these changes

+1 more reviewer

@jcwchen jcwchen jcwchen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.9

Development

Successfully merging this pull request may close these issues.

5 participants

@tomdol @CLAassistant @postrational @jcwchen @fdwr