[MXNET-1215] Allow dynamic shape exists in imperative mode #13283

junrushao · 2018-11-15T13:57:18Z

Description

It is the PR I took over from @zheng-da at #12400. All credits go to Da Zheng.

This PR relaxes the constraint on NDArrays that shape used to be pre-determined. For unittesting, this PR introduces an operator called BooleanMask in contrib, which is a practical use case that actually produces dynamic shape. Note that the support for boolean mask is very experimental, which only allows 2-d inputs, and 1-d mask, because it serves only the functionality of testing. We could improve its better in future PRs.

This is an initial step for the roadmap #12732.

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

Introducing new methods in NDArray allowing delayed assignment of their shapes.
Introducing boolean mask operator, which we believe will be useful in the future.
Add unit test.

Comments

No comments.

TODO

Refactor the boolean index operator
Write backward pass of the boolean index operator
Test coverage

ZhennanQin

Test should be added as well. Overall good approach!

src/operator/contrib/boolean_mask.cc

src/operator/contrib/boolean_mask-inl.h

src/imperative/imperative_utils.h

ZhennanQin · 2018-11-16T01:35:46Z

include/mxnet/ndarray.h

+   * \param dtype data type of this ndarray
+   */
+  explicit NDArray(Context ctx, int dtype = mshadow::default_type_flag) {
+    ptr_ = std::make_shared<Chunk>(TShape(mshadow::Shape1(0)), ctx, true, dtype);


Why not lazily create Chunk when NDArray::Init() is called? Then we don't need to add Chunk::Init() function.

@zheng-da Do you have any specific consideration about this? I am not sure

Because TShape(mshadow::Shape1(0)) doesn't mean no shape. It may confuse other developer, and also cause Chunk shape mismatch with NDArray, which may have potential risk.

The main reason we want to create a chunk here is to create the var in the chunk. Originally, I wanted to allow async execution. Now we only allow sync execution in the imperative mode. We probably don't need to create a chunk here.

Why this PR is merged without approve? I guess this comment isn't addressed. @junrushao1994 @zheng-da @szha

@ZhennanQin Sorry I was too hurry. Personally, I think it is okay just to leave it 0-d for now. In the long term, we could support 0-d tensors in a more systematic approach.

@ZhennanQin we discussed it and think it's ok to use 0-dim tensor for now. Actually, we are using 0-dim shape when the shape is unknown in other places, so it should be fine.

it's true that we should have commented it here as well.

kalyc · 2018-11-16T20:48:23Z

@mxnet-label-bot add [pr-awaiting-review]
@junrushao1994 please take a look at the CI failure and re-trigger CI

src/operator/contrib/boolean_mask.cc

junrushao · 2018-11-18T15:03:21Z

CC: @szha @yidawang @zheng-da

src/operator/contrib/boolean_mask-inl.h

src/operator/contrib/boolean_mask.cc

junrushao · 2018-11-19T02:50:27Z

Have updated according to @zheng-da's comments

junrushao · 2018-11-19T20:26:11Z

I think we are ready to merge this PR. @zheng-da

junrushao · 2018-11-20T07:26:27Z

We merge this PR per our discussion both online and offline, and are aware of the 0-d tensor introduced in this PR. We sincerely thank @ZhennanQin for the very helpful and exemplary comments!

ZhennanQin · 2018-11-20T07:36:50Z

The explanation is OK for me. But it's better to post your offline discussion result in this page before merge, then external reviewer can get involved. Thanks.

junrushao · 2018-11-20T07:41:37Z

@ZhennanQin I agree with every word of your comment. It is completely my fault pushing this PR too hard, mainly because I don't want to block upcoming work. I will definitely behave next time.

…13283)" This reverts commit 779bdc5.

Add NDArray with lazy shape, Add boolean mask operator

84353fb

junrushao requested a review from anirudh2290 as a code owner November 15, 2018 13:57

Fix lints

c3aa226

ZhennanQin reviewed Nov 16, 2018

View reviewed changes

junrushao added 2 commits November 16, 2018 10:45

Address comments and refactor forward pass

822901e

Add backward pass for boolean index

b30cc96

marcoabreu added the pr-awaiting-review PR is waiting for code review label Nov 16, 2018

junrushao added 3 commits November 16, 2018 15:54

Fix tests....

1f61582

Add unittests

de471df

Make lint happy

e3f4c40

zheng-da reviewed Nov 17, 2018

View reviewed changes

src/operator/contrib/boolean_mask.cc Show resolved Hide resolved

Address comments

c092998

zheng-da reviewed Nov 19, 2018

View reviewed changes

src/operator/contrib/boolean_mask-inl.h Outdated Show resolved Hide resolved

zheng-da reviewed Nov 19, 2018

View reviewed changes

src/operator/contrib/boolean_mask-inl.h Show resolved Hide resolved

src/operator/contrib/boolean_mask.cc Outdated Show resolved Hide resolved

src/operator/contrib/boolean_mask.cc Show resolved Hide resolved

Address comments, and complete docstring

2e6a4e6

Update docstring

f634044

szha merged commit 779bdc5 into apache:master Nov 20, 2018

lanking520 added a commit that referenced this pull request Nov 21, 2018

Revert "[MXNET-1215] Allow dynamic shape exists in imperative mode (#…

b114752

…13283)" This reverts commit 779bdc5.

eric-haibin-lin mentioned this pull request Jul 6, 2019

Tutorial for developing advanced operators #15138

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MXNET-1215] Allow dynamic shape exists in imperative mode #13283

[MXNET-1215] Allow dynamic shape exists in imperative mode #13283

junrushao commented Nov 15, 2018 •

edited

Loading

ZhennanQin left a comment

ZhennanQin Nov 16, 2018

junrushao Nov 16, 2018

ZhennanQin Nov 17, 2018

zheng-da Nov 17, 2018

ZhennanQin Nov 20, 2018

junrushao Nov 20, 2018

zheng-da Nov 20, 2018

zheng-da Nov 20, 2018

kalyc commented Nov 16, 2018

junrushao commented Nov 18, 2018

junrushao commented Nov 19, 2018

junrushao commented Nov 19, 2018

junrushao commented Nov 20, 2018 •

edited

Loading

ZhennanQin commented Nov 20, 2018

junrushao commented Nov 20, 2018 •

edited

Loading

[MXNET-1215] Allow dynamic shape exists in imperative mode #13283

[MXNET-1215] Allow dynamic shape exists in imperative mode #13283

Conversation

junrushao commented Nov 15, 2018 • edited Loading

Description

Checklist

Essentials

Changes

Comments

TODO

ZhennanQin left a 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

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

kalyc commented Nov 16, 2018

junrushao commented Nov 18, 2018

junrushao commented Nov 19, 2018

junrushao commented Nov 19, 2018

junrushao commented Nov 20, 2018 • edited Loading

ZhennanQin commented Nov 20, 2018

junrushao commented Nov 20, 2018 • edited Loading

junrushao commented Nov 15, 2018 •

edited

Loading

junrushao commented Nov 20, 2018 •

edited

Loading

junrushao commented Nov 20, 2018 •

edited

Loading