ONNX export: Square and ReduceSum operators by vandanavk · Pull Request #12653 · apache/mxnet

vandanavk · 2018-09-24T19:46:03Z

Description

Enabling export of Square operator and Sum operator

v2: Added reduce_sum tests based on @anirudhacharya's review
v3: Addressed @Roshrini's and @zhreshold's review comments

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 mx.sym.Square using Power operator in ONNX
Export mx.sym.Sum using ReduceSum operator in ONNX
Added tests for ReduceSum

Comments

Tested onnx_backend_test.py

vandanavk · 2018-09-24T20:50:16Z

@mxnet-label-bot[pr-awaiting-review]

anirudhacharya

this should wait on #12633 and please add tests for these operators.

vandanavk · 2018-10-01T21:08:21Z

Rebased on top of #12633.
Added tests for ReduceSum.
Test for square is taken care by the existing tests for Pow operator.

@anirudhacharya @Roshrini @zhreshold This PR is ready for review again.

anirudhacharya · 2018-10-01T22:44:17Z

+    mx_axis = attrs.get("axis", None)
+    axes = convert_string_to_list(str(mx_axis)) if mx_axis is not None else None
+
+    keepdims = 1 if ("keepdims" in attrs) and \


do you need the first condition ("keepdims" in attrs) here?

it's an optional parameter. so added this check just in case.

no, my point was since you are doing attrs.get("keepdims") the first condition is a redundant.

Ok, considering changing it to

keepdims = attrs.get("keepdims", 0) keepdims = 1 if keepdims in ["True", "1"] else 0

I was thinking of this -

keepdims = 1 if attrs.get("keepdims", 0) in ["True", "1"] else 0

but this should be ok i guess.

anirudhacharya · 2018-10-01T22:56:17Z

+    axes = convert_string_to_list(str(mx_axis)) if mx_axis is not None else None
+
+    keepdims = 1 if ("keepdims" in attrs) and \
+                   attrs.get("keepdims") in ["True", "1"] else 0


shouldn't attrs.get("keepdims") be cast to str before making this comparison? I am curious how it passed the unit test.

the value has always been "True" or "1" or "0" when it reaches this function. same is the case with similar functions such as Reduce*, Arg*. I will figure out where this change in type occurs and get back to you.

anirudhacharya · 2018-10-01T23:07:25Z

+            keepdims=keepdims,
+            name=name
+        )
+


nit: could you remove new line here and in line 2150. Or maybe you could have a single return statement after the if else block.

will change it to a common return statement

anirudhacharya · 2018-10-01T23:09:48Z

+        "Pow",
+        [input_node_a, power2_name],
+        [name],
+        name=None


why name=None here?

was a copy from Pow operator. No reason to have it as None. Will add a name. and change it in Pow as well

Looks like name wasn't added to Pow because of the error "pow() got an unexpected keyword argument 'name'". Submitting the fix for this in this PR itself (separate commit).

Roshrini · 2018-10-02T21:36:45Z

LGTM

vandanavk · 2018-10-05T16:34:11Z

@anirudhacharya @zhreshold ping for review

anirudhacharya · 2018-10-05T16:53:53Z

        new_attrs = translation_utils._remove_attributes(new_attrs, ['broadcast'])
        return 'broadcast_power', new_attrs, inputs
-    return 'pow', new_attrs, inputs
+    mxnet_op = symbol.pow(inputs[0], inputs[1])


why this change? you probably made this change to accommodate op_set version 7 of ONNX. will we support op_set versions older than 7?

Reverting back to the older commit. Will figure out supporting different op_set versions and work on the Power operator separately.

vandanavk · 2018-10-08T18:59:26Z

@zhreshold @Roshrini @anirudhacharya review comments addressed. is this PR good to go?

anirudhacharya

lgtm

anirudhacharya · 2018-10-09T00:18:09Z

+    inputs = node["inputs"]
+
+    input_node_a_id = kwargs["index_lookup"][inputs[0][0]]
+


nit: in general feel that there are a lot of unnecessary blank lines in this method. also you can remove line 2096 have np.array([2]) directly. but this change is not a blocker

zhreshold · 2018-10-09T01:11:21Z

+
+    initializer = kwargs["initializer"]
+    power2 = [2]
+    np_arr = np.array(power2)


what is the point of setting power2 as a temporary variable

Agree. Re-submitting after editing this.

zhreshold · 2018-10-09T01:13:12Z

+    axes = convert_string_to_list(str(mx_axis)) if mx_axis is not None else None
+
+    keepdims = attrs.get("keepdims", 0)
+    keepdims = 1 if keepdims in ["True", "1"] else 0


should reuse the function in #12646

Agree. Re-submitting with this change.

vandanavk · 2018-10-09T20:12:25Z

Since this PR uses changes in #12646, combining the 2 PRs. This will be closed in favor of #12646 once the changes are pushed.

vandanavk requested a review from szha as a code owner September 24, 2018 19:46

vandanavk force-pushed the onnx_op branch from 0193f1c to 8a5bbbd Compare September 24, 2018 20:49

marcoabreu added the pr-awaiting-review PR is waiting for code review label Sep 24, 2018

vandanavk force-pushed the onnx_op branch 2 times, most recently from f228e06 to 3d9372d Compare September 25, 2018 18:22

anirudhacharya reviewed Sep 25, 2018

View reviewed changes

vandanavk force-pushed the onnx_op branch from 3d9372d to 2f70a6e Compare October 1, 2018 21:06

anirudhacharya reviewed Oct 1, 2018

View reviewed changes

vandanavk force-pushed the onnx_op branch from 2f70a6e to f8ff6ba Compare October 2, 2018 18:42

anirudhacharya reviewed Oct 5, 2018

View reviewed changes

vandanavk force-pushed the onnx_op branch from f8ff6ba to 2466ad0 Compare October 5, 2018 22:53

anirudhacharya reviewed Oct 9, 2018

View reviewed changes

zhreshold suggested changes Oct 9, 2018

View reviewed changes

ONNX export: Square and sum operators

3c4bc7f

vandanavk force-pushed the onnx_op branch from 2466ad0 to 3c4bc7f Compare October 9, 2018 19:51

vandanavk mentioned this pull request Oct 9, 2018

ONNX export: Fully connected operator w/o bias, ReduceSum, Square #12646

Merged

5 tasks

vandanavk closed this Oct 9, 2018

		inputs = node["inputs"]

		input_node_a_id = kwargs["index_lookup"][inputs[0][0]]

Conversation

vandanavk commented Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Essentials

Changes

Comments

Uh oh!

vandanavk commented Sep 24, 2018

Uh oh!

anirudhacharya left a comment

Choose a reason for hiding this comment

Uh oh!

vandanavk commented Oct 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anirudhacharya Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roshrini commented Oct 2, 2018

Uh oh!

vandanavk commented Oct 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vandanavk commented Oct 8, 2018

Uh oh!

anirudhacharya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vandanavk commented Oct 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vandanavk commented Sep 24, 2018 •

edited

Loading

anirudhacharya Oct 2, 2018 •

edited

Loading