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

ONNX export: Add Crop, Deconvolution and fix the default stride of Pooling to 1 #12399

Merged
merged 2 commits into from
Jan 26, 2019
Merged

ONNX export: Add Crop, Deconvolution and fix the default stride of Pooling to 1 #12399

merged 2 commits into from
Jan 26, 2019

Conversation

ptrendx
Copy link
Member

@ptrendx ptrendx commented Aug 29, 2018 •
edited by Roshrini
Loading

Description

Add operators to ONNX exporter:

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)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@ptrendx ptrendx requested a review from szha as a code owner August 29, 2018 17:24
@vandanavk
Copy link
Contributor

@ptrendx Did you get a chance to test these operators with ONNX? onnx_backend_test.py

@sandeep-krishnamurthy sandeep-krishnamurthy added ONNX pr-awaiting-response PR is reviewed and waiting for contributor to respond labels Aug 30, 2018
@Roshrini
Copy link
Member

@ptrendx Thanks for submitting this PR. :)
You should add 'test_ConvTranspose2d' in "Basic Model Tests" to make sure "Deconvolution" operator is running fine in this file. https://github.com/apache/incubator-mxnet/blob/master/tests/python-pytest/onnx/export/onnx_backend_test.py

Roshrini
Roshrini suggested changes Aug 30, 2018
View reviewed changes
python/mxnet/contrib/onnx/mx2onnx/_op_translations.py Outdated
pad_dims = list(parse_helper(attrs, "pad", [0, 0]))
num_group = int(attrs.get("num_group", 1))
dilations = list(parse_helper(attrs, "dilate", [1, 1]))
adj_dims = list(parse_helper(attrs, "adj"))
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add default value here?

python/mxnet/contrib/onnx/mx2onnx/_op_translations.py Show resolved Hide resolved
anirudhacharya
anirudhacharya suggested changes Aug 31, 2018
View reviewed changes
Copy link
Member

@anirudhacharya anirudhacharya left a comment

Choose a reason for hiding this comment

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

can you add onnx backend tests here -https://github.com/apache/incubator-mxnet/blob/master/tests/python-pytest/onnx/export/onnx_backend_test.py

@sandeep-krishnamurthy
Copy link
Contributor

@ptrendx - ping :-)

@sandeep-krishnamurthy
Copy link
Contributor

@ptrendx - Thanks for your contributions! Can you please address few review comments and we will be good to go?

@vandanavk
Copy link
Contributor

ONNX 1.3 has been merged. Please rebase this PR. Thanks.

@piyushghai piyushghai mentioned this pull request Oct 12, 2018
Closed
@Roshrini
Copy link
Member

@ptrendx Can you please address review comments and rebase this PR? Thanks.

@ptrendx
Copy link
Member Author

ptrendx commented Oct 18, 2018

Hi, I'm sorry for lack of communication on my part. Unfortunately I got pulled into other high priority work and will be able to revisit this PR afterwards.

@roywei
Copy link
Member

roywei commented Oct 29, 2018

Hi @ptrendx , were you able to get some time on this PR? Do you need any help to continue with this?

@kalyc
Copy link
Contributor

kalyc commented Nov 12, 2018

Hi @ptrendx - could you please update on the status for this PR? We are looking forward to merge it into MXNet. Incase if you dont have the bandwidth to address comments on the PR, you can close it at the moment and then re-open it whenever you have the time.
Thanks, please let us know!

@ptrendx
Copy link
Member Author

ptrendx commented Nov 20, 2018

Hi, I'm sorry for the long delay in answering the comments. It's rebased onto the latest master.

vandanavk
vandanavk reviewed Nov 21, 2018
View reviewed changes
python/mxnet/contrib/onnx/mx2onnx/_op_translations.py
"Its definition can change.")

return [crop_node]

Copy link
Contributor

Choose a reason for hiding this comment

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

could you add a test for crop too? - in mxnet_export_test.py maybe

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member Author

Choose a reason for hiding this comment

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

@vandanavk Could you advise where should I put the test for just export? I made the test but am unsure where should it go after the refactor. My understanding is that test_nodes.py is for things that can be both exported and imported, should I put it in mxnet_export_test.py then?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ptrendx export tests usually perform an import-export-import on an ONNX model and then compare inference results.

if you are planning to add an ONNX import for crop, then adding the test would be straightforward - just add a test_case in test_node.py.

if you are adding export alone for crop, then in test_node.py, create a new test case list (same format as the existing maybe), add a new function test_export() in class TestNode(unittest.TestCase), export to onnx format. not sure how to test inference in this case though.

@vandanavk
Copy link
Contributor

vandanavk commented Nov 27, 2018 •
edited
Loading

@mxnet-label-bot update [pr-awaiting-testing, ONNX]

@marcoabreu marcoabreu added pr-awaiting-testing PR is reviewed and waiting CI build and test ONNX and removed ONNX pr-awaiting-response PR is reviewed and waiting for contributor to respond labels Nov 27, 2018
Roshrini
Roshrini suggested changes Dec 7, 2018
View reviewed changes
Copy link
Member

@Roshrini Roshrini left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments. Added minor comments. Can you please take care of these as well? Thanks!

python/mxnet/contrib/onnx/mx2onnx/_op_translations.py Outdated Show resolved Hide resolved
python/mxnet/contrib/onnx/mx2onnx/_op_translations.py
"Its definition can change.")

return [crop_node]

Copy link
Member

Choose a reason for hiding this comment

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

+1

@Roshrini Roshrini mentioned this pull request Dec 7, 2018
Closed
@Roshrini
Copy link
Member

@ptrendx Can you address the review comments so that we can merge this PR? :)

@Roshrini
Copy link
Member

Roshrini commented Jan 2, 2019

@ptrendx Can you please rebase this PR?

@ptrendx ptrendx force-pushed the onnx_add_deconvolution_crop branch from ba0b25e to abd6951 Compare January 3, 2019 22:11
@stu1130
Copy link
Contributor

stu1130 commented Jan 15, 2019

@Roshrini could you take a look at this?Thanks

@Roshrini
Copy link
Member

Hi @ptrendx, if you have test ready for crop, can you please add it? Or maybe create a separate PR for crop operator? Other part of this PR looks good and is ready to be merged.

@Roshrini
Copy link
Member

Thank you for working on this ops @ptrendx :) I will add test case for crop in my PR as I am already adding tests for export ops in this PR: #13981

@Roshrini Roshrini merged commit 25e915b into apache:master Jan 26, 2019
jessr92 pushed a commit to jessr92/incubator-mxnet that referenced this pull request Jan 27, 2019
@ptrendx
ONNX export: Add Crop, Deconvolution and fix the default stride of Po…
b43da69 
…oling to 1 (apache#12399)

* Added Deconvolution and Crop to ONNX exporter

* Added default for pool_type
jessr92 pushed a commit to jessr92/incubator-mxnet that referenced this pull request Jan 27, 2019
@ptrendx
ONNX export: Add Crop, Deconvolution and fix the default stride of Po…
7ccc12f 
…oling to 1 (apache#12399)

* Added Deconvolution and Crop to ONNX exporter

* Added default for pool_type
stephenrawls pushed a commit to stephenrawls/incubator-mxnet that referenced this pull request Feb 16, 2019
@ptrendx @stephenrawls
ONNX export: Add Crop, Deconvolution and fix the default stride of Po…
ae12ee5 
…oling to 1 (apache#12399)

* Added Deconvolution and Crop to ONNX exporter

* Added default for pool_type
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@ptrendx
ONNX export: Add Crop, Deconvolution and fix the default stride of Po…
5ce77f2 
…oling to 1 (apache#12399)

* Added Deconvolution and Crop to ONNX exporter

* Added default for pool_type
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@anirudhacharya anirudhacharya anirudhacharya requested changes

@Roshrini Roshrini Roshrini approved these changes

@vandanavk vandanavk vandanavk approved these changes

@szha szha Awaiting requested review from szha szha is a code owner

Assignees
No one assigned
Labels
ONNX pr-awaiting-testing PR is reviewed and waiting CI build and test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No conversion function registered for op type Deconvolution yet
9 participants
@ptrendx @vandanavk @Roshrini @sandeep-krishnamurthy @roywei @kalyc @stu1130 @anirudhacharya @marcoabreu