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 test bugs for resize op version 11 #2425

Merged
merged 1 commit into from
Oct 30, 2019

Conversation

askhade
Copy link
Contributor

@askhade askhade commented Oct 29, 2019

Some of the tests have bugs where the model is not given the expected inputs.

For example test : resize_upsample_sizes_nearest_ceil_half_pixel
Expects the results with rounding mode as ceil however the model is not given this as input therefore during inferencing backends end up using the default rounding mode and the test fails.

@askhade askhade requested a review from a team as a code owner October 29, 2019 19:48
@askhade
Copy link
Contributor Author

askhade commented Oct 30, 2019

@daquexian : I noticed the formula for coordinate transformation which is specified in the spec

x_original = length_resized > 1 ? (x_resized + 0.5) / scale - 0.5 : 0, <br/>
is different than the one in the interpolate_1d method used in resize.py
if output_width == 1:


Can you clarify why it is different?

Either ways we need to change one so that they both match... If the change is needed in resize.py then I can add it to this PR... if it is required in defs.cc then we will need an opset bump

@daquexian
Copy link
Member

Nice catch. I think the changes is needed in defs.cc. The implementation in resize.py has been verified to be compatible with PyTorch here

@askhade
Copy link
Contributor Author

askhade commented Oct 30, 2019

Nice catch. I think the changes is needed in defs.cc. The implementation in resize.py has been verified to be compatible with PyTorch here

This means we need to update the opset version for this change... I will include it as part of another PR.

There are more changes required in defs.cc for example :

Here scales is meant to be optional but it is not declared as optional... "OpSchema::Optional" needs to be explicitly specified for any optional inputs.

" be the same as the rank of input 'X'. Only one of 'scales' and 'sizes' can be specified. If 'size' is needed, the user can use an empty string as the name of 'scales' in this operator's input list.",

I am planning to make these changes as part of the next PR and bump up the opset version

@ebarsoum ebarsoum merged commit 79bd504 into onnx:master Oct 30, 2019
@winnietsang winnietsang added this to the 1.7 milestone Feb 12, 2020
jcwchen pushed a commit to jcwchen/onnx that referenced this pull request Sep 23, 2020
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.

4 participants