Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

fix for params with no dims in onnx #13413

Merged
merged 7 commits into from
Jan 9, 2019
Merged

Conversation

Roshrini
Copy link
Member

@Roshrini Roshrini commented Nov 26, 2018

Description

input tensor_proto can be a numeric variable and sometimes it can have tensor_proto.dims parameter empty. In this case, as the shape cannot be infered, and reshape will throw an error. Adding a fix for it

Fixes #13300
@thomelane @vandanavk

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http:https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@Roshrini Roshrini requested a review from szha as a code owner November 26, 2018 22:29
@Roshrini
Copy link
Member Author

@wangxinanbook added a fix for the issue in this PR as this issue came while running a model without dims value in params.
FIxes #13300

@Roshrini Roshrini changed the title fix for params with no dims fix for params with no dims in onnx Nov 26, 2018
@vandanavk
Copy link
Contributor

@mxnet-label-bot add [pr-awaiting-review, ONNX]

@marcoabreu marcoabreu added ONNX pr-awaiting-review PR is waiting for code review labels Nov 27, 2018
Copy link
Contributor

@vandanavk vandanavk left a comment

Choose a reason for hiding this comment

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

Please add Fixes #13300 in the PR description

np_array = to_array(tensor_proto).reshape(tuple(tensor_proto.dims))
else:
np_array = np.array([to_array(tensor_proto)])
return nd.array(np_array)
Copy link
Contributor

Choose a reason for hiding this comment

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

this line should be outside the else

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@Roshrini Roshrini force-pushed the no_dims_params branch 2 times, most recently from da32f75 to 17b14f0 Compare December 7, 2018 18:43
Copy link
Contributor

@vandanavk vandanavk left a comment

Choose a reason for hiding this comment

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

LGTM

@nswamy
Copy link
Member

nswamy commented Dec 8, 2018

do you think it makes sense to add a unit test for this case?

@Roshrini
Copy link
Member Author

Roshrini commented Jan 9, 2019

@nswamy Added test case

@codecov-io
Copy link

Codecov Report

Merging #13413 into master will decrease coverage by 2.03%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13413      +/-   ##
==========================================
- Coverage   83.27%   81.24%   -2.04%     
==========================================
  Files         722      724       +2     
  Lines       79010    80525    +1515     
  Branches     3223     3229       +6     
==========================================
- Hits        65796    65420     -376     
- Misses      12344    14232    +1888     
- Partials      870      873       +3
Impacted Files Coverage Δ
python/mxnet/contrib/onnx/onnx2mx/import_onnx.py 12.39% <25%> (+0.53%) ⬆️
src/operator/quantization/quantized_conv.cu 1.94% <0%> (-91.27%) ⬇️
...operator/quantization/quantized_fully_connected.cu 6.89% <0%> (-86.21%) ⬇️
src/operator/quantization/quantized_pooling.cu 4% <0%> (-82%) ⬇️
src/operator/contrib/multibox_prior.cu 2.7% <0%> (-81.09%) ⬇️
src/operator/contrib/multibox_detection.cu 4.76% <0%> (-80.96%) ⬇️
src/operator/crop.cu 33.33% <0%> (-66.67%) ⬇️
src/operator/crop-inl.h 13.59% <0%> (-59.68%) ⬇️
src/common/static_array.h 50% <0%> (-50%) ⬇️
src/operator/pooling_v1-inl.h 13.4% <0%> (-45.45%) ⬇️
... and 270 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe46cd9...16e107c. Read the comment docs.

@nswamy nswamy merged commit ed92b8d into apache:master Jan 9, 2019
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
* fix for params with no dims

* fix

* fix

* retrigger build

* test added

* retrigger CI

* retrigger ci
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ONNX pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data type error when import onnx model
5 participants