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

fix for asymmetric padding #10676

Merged
merged 6 commits into from
Apr 25, 2018
Merged

fix for asymmetric padding #10676

merged 6 commits into from
Apr 25, 2018

Conversation

anirudhacharya
Copy link
Member

@anirudhacharya anirudhacharya commented Apr 24, 2018

Description

Fixes asymmetric padding issue in onnx-mxnet translators. MXNet conv operator supports only symmetric padding, whereas the conv operator in ONNX can have asymmetric padding. We are fixing this discrepancy by sequencing MXNet's 'Pad' operator and MXNet's 'Convolution' operator. Because the 'Pad' operator in MXNet supports asymmetric padding.

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: The 'Pad' operator has tests for both symmetric and asymmetric padding.
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Comments

Copy link
Contributor

@rajanksin rajanksin left a comment

Choose a reason for hiding this comment

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

Could you add some comments, why we are doing padding before conv

Copy link
Contributor

@rajanksin rajanksin left a comment

Choose a reason for hiding this comment

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

LGTM

@zheng-da
Copy link
Contributor

Is there a unit test to cover this?

@anirudhacharya
Copy link
Member Author

The CI failed in one of the flaky tests while trying to fetch test files -
ProtocolError: ("Connection broken: ConnectionResetError(104, 'Connection reset by peer')", ConnectionResetError(104, 'Connection reset by peer'))
Will rerun the CI.

@anirudh2290
Copy link
Member

@marcoabreu I see that tutorials test have been added to the CI. How can I find the timeout set for the tutorials job ?

@anirudhacharya
Copy link
Member Author

@marcoabreu the last two CI runs failed on the following tests -

  • test_tutorials.test_onnx_inference_on_onnx_model (tutorial tests Python 3 GPU)
  • test_tutorials.test_onnx_fine_tuning_gluon (tutorial tests Python 2 GPU)

both due to "Connection reset". What is the timeout set for these tests? Could you please take a look.

@anirudh2290
Copy link
Member

Okay it seems to be set in test_tutorials to 7 mins: https://github.com/apache/incubator-mxnet/blob/master/tests/tutorials/test_tutorials.py#L49

@anirudh2290
Copy link
Member

@piiswrong @eric-haibin-lin is this good to merge ?

@piiswrong piiswrong merged commit 0cd082b into apache:master Apr 25, 2018
anirudh2290 pushed a commit to anirudh2290/mxnet that referenced this pull request Apr 25, 2018
@anirudh2290 anirudh2290 mentioned this pull request Apr 25, 2018
7 tasks
piiswrong pushed a commit that referenced this pull request Apr 25, 2018
@anirudhacharya anirudhacharya deleted the fix branch April 26, 2018 03:20
rahul003 pushed a commit to rahul003/mxnet that referenced this pull request Jun 4, 2018
* fix for asymmetric padding

* add asymmetric support

* lint and code styling and more comments.

* change bias to boolean value.

* update timeout to 10 min per test.
ijkguo pushed a commit to ijkguo/mxnet that referenced this pull request Jun 5, 2018
zheng-da pushed a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 28, 2018
* fix for asymmetric padding

* add asymmetric support

* lint and code styling and more comments.

* change bias to boolean value.

* update timeout to 10 min per test.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants