[MXNET-886] ONNX export: HardSigmoid, Less, Greater, Equal #12812

vandanavk · 2018-10-12T16:10:34Z

Description

Added support for exporting HardSigmoid, Less, Greater, Equal to ONNX.

v2: Addressed @anirudhacharya's comments on the tests for comparison operators.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
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://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

Changes

Export the 4 operators in _op_translations.py
Added tests for the 4 operators

Comments

sandeep-krishnamurthy · 2018-10-12T19:57:46Z

@Roshrini @anirudhacharya

anirudhacharya · 2018-10-12T20:56:59Z

tests/python-pytest/onnx/export/mxnet_export_test.py


 if __name__ == '__main__':
+ test_greater()


do not call each of the test methods separately. These tests are written based on pytest framework. You can say unittest.main(). pytest at runtime will collect the tests that need to be run.

anirudhacharya · 2018-10-12T21:15:32Z

tests/python-pytest/onnx/export/mxnet_export_test.py

+ """Test for logical greater in onnx operators."""
+ input1 = np.array([[ 1, 1, 1], [ 1, 1, 1]]).astype("float32")
+ input2 = np.array([[ 0], [ 1]]).astype("float32")
+ op = np.array([[ 1, 1, 1], [ 0, 0, 0]]).astype("float32")


do not hard code this operation. use np.greater. Also can you please name this variable different, like expected_value or numpy_op or something like that.

anirudhacharya · 2018-10-12T21:16:07Z

tests/python-pytest/onnx/export/mxnet_export_test.py

+ """Test for equal in onnx operators."""
+ input1 = np.array([[ 1, 1, 1], [ 1, 1, 1]]).astype("float32")
+ input2 = np.array([[ 0], [ 1]]).astype("float32")
+ op = np.array([[ 0, 0, 0], [ 1, 1, 1]]).astype("float32")


do not hard code this operation. use np.equal. Also can you please name this variable different, like expected_value or numpy_op or something like that.

anirudhacharya · 2018-10-12T21:16:21Z

tests/python-pytest/onnx/export/mxnet_export_test.py

+ """Test for lesser in onnx operators."""
+ input1 = np.array([[ 1, 1, 1], [ 1, 1, 1]]).astype("float32")
+ input2 = np.array([[ 0], [ 1]]).astype("float32")
+ op = np.array([[ 0, 0, 0], [ 0, 0, 0]]).astype("float32")


do not hard code this operation. use np.lesser. Also can you please name this variable different, like expected_value or numpy_op or something like that.

Roshrini

Thanks Vandana for adding these operators. Added few comments.

Also make sure you add this "tests/python-pytest/onnx/export/mxnet_export_test.py" file in CI

python/mxnet/contrib/onnx/mx2onnx/_op_translations.py

python/mxnet/contrib/onnx/mx2onnx/export_onnx.py

vandanavk · 2018-10-16T21:19:06Z

Thanks @Roshrini. Made the changes you suggested and submitted

Roshrini · 2018-10-16T21:43:11Z

Thanks for fixing :) LGTM

anirudhacharya

LGTM

tests/python-pytest/onnx/export/mxnet_export_test.py

vandanavk · 2018-10-16T23:16:31Z

@zhreshold for review

zhreshold · 2018-10-16T23:53:34Z

python/mxnet/contrib/onnx/mx2onnx/export_onnx.py

- test_mod.init_params()
- else:
- test_mod.set_params(arg_params=arg_params, aux_params=aux_params, allow_missing=True)
+ # module bind method requires all data to have same batch size,


why not use sym.bind whatsoever

tests for batchnorm and conv2d alone fail due to missing keys. will debug further and update.

@zhreshold Will debug this and submit a separate PR for this commit and InstanceNorm (which depends on this). This will make sure that HardSigmoid and the comparison operators can be reviewed/merged instead of waiting on this. Hope that's ok.

@vandanavk making sure only high quality code get merged into master is critical. Having pushing feature request isn't a good reason to bypass this restriction.

Agreed @zhreshold. Opened a WIP PR #12920 to continue debug.

The changes in this PR on HardSigmoid and comparison operators are not impacted by the changes in PR #12920.

zhreshold · 2018-10-16T23:53:51Z

tests/python-pytest/onnx/export/backend_rep.py

- mod.bind(for_training=False, data_shapes=data_shapes,
- label_shapes=None)
- mod.set_params(arg_params=self.arg_params, aux_params=self.aux_params)
+ # module bind method requires all data to have same batch size,


duplicate in onnx_export.py

vandanavk · 2018-10-29T17:24:00Z

@zhreshold Please let me know if there are any other inputs on this PR. If this PR is good to go, I can rebase my other PRs on top of this.

zhreshold · 2018-10-29T18:07:38Z

@vandanavk What is the PR that is going to correct the repetitive importing?

vandanavk · 2018-10-29T18:19:27Z

@zhreshold This PR #12878

zhreshold · 2018-11-01T18:32:14Z

please rebase to changes in #12878

vandanavk · 2018-11-01T19:45:38Z

Rebased. Ready for review again

ankkhedia · 2018-11-02T23:04:35Z

@zhreshold @sandeep-krishnamurthy @nswamy Could you please help with review/merge?

nswamy · 2018-11-04T17:34:24Z

tests/python-pytest/onnx/export/mxnet_export_test.py

@@ -238,6 +238,63 @@ def test_square():

 npt.assert_almost_equal(result, numpy_op)

+@with_seed()
+def test_greater():


I am seeing that a lot of repetitions in the test code. Could you please modularize the test code? happy to discuss offline

@nswamy @vandanavk was the concern addressed offline? what's the status of this PR?

python/mxnet/contrib/onnx/mx2onnx/_op_translations.py

vandanavk requested a review from szha as a code owner October 12, 2018 16:10

vandanavk changed the title ~~ONNX export: HardSigmoid, Less, Greater, Equal~~ [MXNET-886] ONNX export: HardSigmoid, Less, Greater, Equal Oct 12, 2018

vandanavk changed the title ~~[MXNET-886] ONNX export: HardSigmoid, Less, Greater, Equal~~ [MXNET-886] ONNX export: HardSigmoid, Less, Greater, Equal, InstanceNorm Oct 12, 2018

vandanavk force-pushed the onnx_op branch 2 times, most recently from d779756 to 9d40dff Compare October 12, 2018 18:45

sandeep-krishnamurthy added ONNX pr-awaiting-review PR is waiting for code review labels Oct 12, 2018

anirudhacharya reviewed Oct 12, 2018

View reviewed changes

vandanavk force-pushed the onnx_op branch 3 times, most recently from f9ba301 to ae66c1c Compare October 16, 2018 00:01

Roshrini suggested changes Oct 16, 2018

View reviewed changes

python/mxnet/contrib/onnx/mx2onnx/_op_translations.py Outdated Show resolved Hide resolved

python/mxnet/contrib/onnx/mx2onnx/export_onnx.py Outdated Show resolved Hide resolved

vandanavk force-pushed the onnx_op branch from ae66c1c to 24063cc Compare October 16, 2018 21:18

vandanavk force-pushed the onnx_op branch from 24063cc to 7b0217c Compare October 16, 2018 21:41

anirudhacharya reviewed Oct 16, 2018

View reviewed changes

tests/python-pytest/onnx/export/mxnet_export_test.py Outdated Show resolved Hide resolved

vandanavk force-pushed the onnx_op branch from 7b0217c to 77c8a87 Compare October 16, 2018 23:01

zhreshold suggested changes Oct 16, 2018

View reviewed changes

vandanavk force-pushed the onnx_op branch from 77c8a87 to 2be59e8 Compare October 23, 2018 00:39

vandanavk changed the title ~~[MXNET-886] ONNX export: HardSigmoid, Less, Greater, Equal, InstanceNorm~~ [MXNET-886] ONNX export: HardSigmoid, Less, Greater, Equal Oct 23, 2018

vandanavk force-pushed the onnx_op branch from 2be59e8 to af3f8b3 Compare October 23, 2018 04:20

vandanavk force-pushed the onnx_op branch from af3f8b3 to 8363ef9 Compare November 1, 2018 18:47

vandanavk force-pushed the onnx_op branch 4 times, most recently from 37aa0aa to cb5e291 Compare November 2, 2018 19:36

zhreshold approved these changes Nov 2, 2018

View reviewed changes

nswamy reviewed Nov 4, 2018

View reviewed changes

vandanavk force-pushed the onnx_op branch 2 times, most recently from 77e0bf1 to 9d440cf Compare November 20, 2018 22:51

nswamy approved these changes Nov 20, 2018

View reviewed changes

python/mxnet/contrib/onnx/mx2onnx/_op_translations.py Show resolved Hide resolved

python/mxnet/contrib/onnx/mx2onnx/_op_translations.py Outdated Show resolved Hide resolved

nswamy added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-review PR is waiting for code review labels Nov 20, 2018

vandanavk force-pushed the onnx_op branch 4 times, most recently from 5cc157e to de4c390 Compare November 26, 2018 17:48

vandanavk added 2 commits November 26, 2018 11:49

ONNX export: Hard sigmoid

1ca7170

ONNX export: Comparison operators

5f2c5c3

vandanavk force-pushed the onnx_op branch from de4c390 to 5f2c5c3 Compare November 26, 2018 19:51

sandeep-krishnamurthy approved these changes Nov 29, 2018

View reviewed changes

sandeep-krishnamurthy merged commit f876461 into apache:master Nov 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MXNET-886] ONNX export: HardSigmoid, Less, Greater, Equal #12812

[MXNET-886] ONNX export: HardSigmoid, Less, Greater, Equal #12812

vandanavk commented Oct 12, 2018 •

edited

Loading

sandeep-krishnamurthy commented Oct 12, 2018

anirudhacharya Oct 12, 2018

anirudhacharya Oct 12, 2018

anirudhacharya Oct 12, 2018

anirudhacharya Oct 12, 2018

Roshrini left a comment

vandanavk commented Oct 16, 2018

Roshrini commented Oct 16, 2018

anirudhacharya left a comment

vandanavk commented Oct 16, 2018

zhreshold Oct 16, 2018

vandanavk Oct 22, 2018

vandanavk Oct 23, 2018

zhreshold Oct 23, 2018

vandanavk Oct 23, 2018

zhreshold Oct 16, 2018

vandanavk commented Oct 29, 2018

zhreshold commented Oct 29, 2018

vandanavk commented Oct 29, 2018

zhreshold commented Nov 1, 2018

vandanavk commented Nov 1, 2018

ankkhedia commented Nov 2, 2018

nswamy Nov 4, 2018

stu1130 Nov 20, 2018

[MXNET-886] ONNX export: HardSigmoid, Less, Greater, Equal #12812

[MXNET-886] ONNX export: HardSigmoid, Less, Greater, Equal #12812

Conversation

vandanavk commented Oct 12, 2018 • edited Loading

Description

Checklist

Essentials

Changes

Comments

sandeep-krishnamurthy commented Oct 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roshrini left a comment

Choose a reason for hiding this comment

vandanavk commented Oct 16, 2018

Roshrini commented Oct 16, 2018

anirudhacharya left a comment

Choose a reason for hiding this comment

vandanavk commented Oct 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vandanavk commented Oct 29, 2018

zhreshold commented Oct 29, 2018

vandanavk commented Oct 29, 2018

zhreshold commented Nov 1, 2018

vandanavk commented Nov 1, 2018

ankkhedia commented Nov 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vandanavk commented Oct 12, 2018 •

edited

Loading