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

Add more support for mxnet_to_coreml #14222

Merged
merged 6 commits into from Mar 6, 2019
Merged

Add more support for mxnet_to_coreml #14222

merged 6 commits into from Mar 6, 2019

Conversation

wagamama
Copy link
Contributor

@wagamama wagamama commented Feb 21, 2019 •
edited

  • Add PReLU
  • Add group support in Convolution
  • Skip _minus_scalar and _mul_scalar

Description

Add more support for mxnet_to_coreml

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Comments

@wagamama
Add support of mxnet_to_coreml
2bc5a59 
 * Add PReLU
 * Add group support in Convolution
 * Skip _minus_scalar and _mul_scalar
@wagamama wagamama requested a review from szha as a code owner February 21, 2019 07:33
@szha
Copy link
Member

szha commented Feb 22, 2019

@pracheer could you help take a look? Thanks.

@Roshrini Roshrini added pr-awaiting-review PR is waiting for code review CoreML Issues related to CoreML converter labels Feb 22, 2019
pracheer
pracheer suggested changes Feb 24, 2019
View reviewed changes
Copy link
Contributor

@pracheer pracheer 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 the change. It'll be great to have some unit tests for the cases you are adding.
Also added a couple of extremely minor (nitpick) comments.

tools/coreml/converter/_layers.py Show resolved Hide resolved
tools/coreml/converter/_layers.py Show resolved Hide resolved
tools/coreml/converter/_layers.py Show resolved Hide resolved
tools/coreml/converter/_layers.py Outdated Show resolved Hide resolved
@wagamama
Comply with code style
a1e937d 
* Add spare line between comment and code
* Add two spare lines between functions
* Change "Convert an leakyrelu" to "Convert a leakyrelu"
@wagamama
Check if num_group exists in param before assigning value
cb98067
@wagamama
Add test cases for test_mxnet_converter
c94bbb8 
* Add test_tiny_prelu_leakyrelu_random_input
* Add test_really_tiny_conv_random_input_multi_group
* Add test_tiny_conv_random_input_multi_group
* Modify test_conv_random by adding num_group
pracheer
pracheer suggested changes Feb 27, 2019
View reviewed changes
Copy link
Contributor

@pracheer pracheer 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 adding the test cases.
Couple of relatively minor comments otherwise the change looks good.

tools/coreml/test/test_mxnet_converter.py Outdated Show resolved Hide resolved
tools/coreml/test/test_mxnet_converter.py Show resolved Hide resolved
@wagamama
Rollback to not setting num_group
0189933
@wagamama
Add "leaky" and "elu" in converter
da9f7cb 
* Add support of "leaky" and "elu"
* Add unit test for "leaky" and "elu"
pracheer
pracheer approved these changes Mar 1, 2019
View reviewed changes
Copy link
Contributor

@pracheer pracheer 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 the changes!

vandanavk
vandanavk approved these changes Mar 4, 2019
View reviewed changes
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

@szha szha merged commit fccce20 into apache:master Mar 6, 2019
vdantu pushed a commit to vdantu/incubator-mxnet that referenced this pull request Mar 31, 2019
@wagamama @vdantu
Add more support for mxnet_to_coreml (apache#14222)
affbf98 
* Add support of mxnet_to_coreml

 * Add PReLU
 * Add group support in Convolution
 * Skip _minus_scalar and _mul_scalar

* Comply with code style

* Add spare line between comment and code
* Add two spare lines between functions
* Change "Convert an leakyrelu" to "Convert a leakyrelu"

* Check if num_group exists in param before assigning value

* Add test cases for test_mxnet_converter

* Add test_tiny_prelu_leakyrelu_random_input
* Add test_really_tiny_conv_random_input_multi_group
* Add test_tiny_conv_random_input_multi_group
* Modify test_conv_random by adding num_group

* Rollback to not setting num_group

* Add "leaky" and "elu" in converter

* Add support of "leaky" and "elu"
* Add unit test for "leaky" and "elu"
nswamy pushed a commit that referenced this pull request Apr 5, 2019
@wagamama @nswamy
Add more support for mxnet_to_coreml (#14222)
e64cfaa 
* Add support of mxnet_to_coreml

 * Add PReLU
 * Add group support in Convolution
 * Skip _minus_scalar and _mul_scalar

* Comply with code style

* Add spare line between comment and code
* Add two spare lines between functions
* Change "Convert an leakyrelu" to "Convert a leakyrelu"

* Check if num_group exists in param before assigning value

* Add test cases for test_mxnet_converter

* Add test_tiny_prelu_leakyrelu_random_input
* Add test_really_tiny_conv_random_input_multi_group
* Add test_tiny_conv_random_input_multi_group
* Modify test_conv_random by adding num_group

* Rollback to not setting num_group

* Add "leaky" and "elu" in converter

* Add support of "leaky" and "elu"
* Add unit test for "leaky" and "elu"
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@wagamama
Add more support for mxnet_to_coreml (apache#14222)
4545b1f 
* Add support of mxnet_to_coreml

 * Add PReLU
 * Add group support in Convolution
 * Skip _minus_scalar and _mul_scalar

* Comply with code style

* Add spare line between comment and code
* Add two spare lines between functions
* Change "Convert an leakyrelu" to "Convert a leakyrelu"

* Check if num_group exists in param before assigning value

* Add test cases for test_mxnet_converter

* Add test_tiny_prelu_leakyrelu_random_input
* Add test_really_tiny_conv_random_input_multi_group
* Add test_tiny_conv_random_input_multi_group
* Modify test_conv_random by adding num_group

* Rollback to not setting num_group

* Add "leaky" and "elu" in converter

* Add support of "leaky" and "elu"
* Add unit test for "leaky" and "elu"
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pracheer pracheer pracheer 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
CoreML Issues related to CoreML converter pr-awaiting-review PR is waiting for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@wagamama @szha @pracheer @vandanavk @Roshrini