[MXNET-791] Pick with negative indices #12090

perdasilva · 2018-08-08T16:38:35Z

Description

Updates pick operator to handle negative indices.
Implements #12009

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

Updates pick and pick_grad in broadcast_reduce_op to handle negative indices
Updates pick to include a mode parameter ('pick' or 'wrap') to specify whether out of bounds indices should either be clipped or wrapped.

Comments

Making change as it was tagged with Call for Contribution

perdasilva · 2018-08-08T16:45:04Z

Hi,

This is my first PR to the project and I'm still learning the ropes. Any and all suggestions would be much welcome. I'm not sure how to handle the case. For simplicity, I handled it with mod arithmetic. But in numpy, you can index in the range [-len, len). Which behaviour should be correct for this issue?
Should the cropping happen for indices < -1*len and >= len ?

Cheers,

Per

perdasilva · 2018-08-08T16:54:39Z

tests/python/unittest/test_operator.py

@@ -30,6 +30,41 @@
 from common import setup_module, with_seed, teardown, assert_raises_cudnn_disabled, assertRaises
 import unittest

+@with_seed()


I will move this back to its proper place - didn't get a chance as my family arrived home =P

haojin2 · 2018-08-08T18:16:36Z

Welcome to MXNet family!
I think you can use what I did for take operator as a reference: https://github.com/apache/incubator-mxnet/pull/11326/files#diff-ed06b8d9798aca630313f2a9dd3fcd68R316 for handling negative indices.

haojin2 · 2018-08-08T19:52:24Z

src/operator/tensor/broadcast_reduce_op_index.cc

@@ -131,7 +130,7 @@ Examples::
  pick(x, y=[0,1], 0) = [ 1.,  4.]

  // picks elements with specified indices along axis 1
-  pick(x, y=[0,1,0], 1) = [ 1.,  4.,  5.]
+  pick(x, y=[0,-1,0], 1) = [ 1.,  4.,  5.]


Please consider adding extra example for the newly supported case instead of modifying the existing cases.
For this case maybe you can do

pick(x, y=[0,1,0,-1], 1) = [ 1., 4., 5., 4.]

haojin2 · 2018-08-08T19:52:52Z

tests/python/unittest/test_operator.py

@@ -4417,7 +4417,7 @@ def test_pick_helper(index_type=np.int32):
            sshape = bshape.copy()
            sshape[axis] = 1
            data = np.random.uniform(-1, 1, size=bshape)
-            index = np.random.randint(0, bshape[axis], size=sshape)
+            index = np.random.randint(-1*bshape[axis], bshape[axis], size=sshape)


I would prefer -bshape[axis].

perdasilva · 2018-08-09T14:44:45Z

Thank you for the welcome and guidance ^^
I hope I have done everything correctly. I've incorporated the mode into the currently existing pick test. I wonder if they should be split? Or maybe the pick_helper should be extended with the mode?

Cheers!!

Per

haojin2 · 2018-08-09T17:08:21Z

src/operator/tensor/broadcast_reduce_op.h

@@ -99,6 +99,10 @@ struct ReduceAxisParam : public dmlc::Parameter<ReduceAxisParam> {
  }
 };

+namespace pick_ {  // to avoid name conflict
+enum TakeOpMode {kRaise, kWrap, kClip};


I would prefer this to be like

enum PickOpMode {kWrap, kClip};

as we do not have raise mode at all.

This way you do not need the extra namespace either.

haojin2 · 2018-08-09T17:13:18Z

tests/python/unittest/test_operator.py

@@ -4417,7 +4417,12 @@ def test_pick_helper(index_type=np.int32):
            sshape = bshape.copy()
            sshape[axis] = 1
            data = np.random.uniform(-1, 1, size=bshape)
-            index = np.random.randint(0, bshape[axis], size=sshape)
+            mode = np.random.choice(a=['clip', 'wrap']) 


Please test both choices for mode using a for-loop instead of randomized testing for mode

haojin2 · 2018-08-09T17:14:01Z

src/operator/tensor/broadcast_reduce_op_index.cc

@@ -117,7 +117,7 @@ an output array of shape ``(i0,)`` with::
  output[i] = input[i, indices[i]]

 By default, if any index mentioned is too large, it is replaced by the index that addresses
-the last element along an axis (the `clip` mode).
+the last element along an axis (the `clip` mode). 


Please get rid of the unnecessary extra whitespace on this line.

perdasilva · 2018-08-09T17:38:59Z

Will do. I kept the raise there to be consistent with the take implementation. Would you like me to remove raise from take?

perdasilva · 2018-08-10T06:52:23Z

Do you think it would make sense to test 'wrap' with the range -2*-bshape[axis]..2*bshape[axis]? To cover cases where one is wrapping more than the length of the axis in both directions? In my understanding, in numpy indices should generally be [-len, len). So, the current implementation diverges from that.

haojin2 · 2018-08-10T07:29:42Z

@perdasilva Sorry for the late reply, I think adding more coverage is totally okay. Regarding the inconsistency with numpy, does this operator has some equivalent op in numpy? There is one for take, but this one I'm not sure.

perdasilva · 2018-08-10T08:00:41Z

@haojin2 No problem at all ^^
The only thing I could find that is similar to pick in numpy is numpy.choose. But there's no axis input there. It also seems that 'raise' is the default mode. Upon closer inspection, 'wrap' works the same here as with numpy. Interestingly though, np.array indexing is in the range [-len, len) ^^

I'll also increase the coverage for the test.

haojin2 · 2018-08-10T20:28:30Z

@perdasilva So if there's no direct equivalence of this one in numpy then we may not need to match the behavior with numpy, please simply ensure we have enough documentation to document the behavior.

perdasilva · 2018-08-11T10:25:47Z

I believe that's all done now. Thanks for everything ^^

perdasilva · 2018-08-14T06:35:36Z

@haojin2 I believe everything is done as specified. Please merge when you get a chance ^^

sandeep-krishnamurthy

Overall LGTM.
Thank you @perdasilva for the contributions and @haojin2 for reviewing the code.

I see dmlc-core is being updated with this PR. Is it due to recent revert @haojin2 ? How do we proceed here

sandeep-krishnamurthy · 2018-08-15T23:47:44Z

src/operator/tensor/broadcast_reduce_op.h

+    .add_enum("wrap", kWrap)
+    .add_enum("clip", kClip)
+    .set_default(kClip)
+    .describe("Specify how out-of-bound indices bahave. Default is \"clip\"."


nit: bahave-> behave

Good catch! Thank you!

haojin2 · 2018-08-15T23:59:40Z

Please rebase with the latest master, then do a git submodule update --init --recursive, then do a git push --force origin <your-branch-name>

perdasilva · 2018-08-16T06:20:37Z

@haojin2 @sandeep-krishnamurthy I've fixed the typo and done as hao jin asked. Thanks for the review guys ^^

perdasilva · 2018-08-16T13:49:15Z

Hmm those dmlc changes seem to have persisted...strange

perdasilva · 2018-08-16T14:00:50Z

Ok - Just did a soft reset, updated the submodule, then re-applied my commits. Seems to have fixed the issue =)

sandeep-krishnamurthy

Thanks @perdasilva
LGTM.

sandeep-krishnamurthy · 2018-08-17T15:45:19Z

@haojin2 - This PR is ready for merge after your approval.

haojin2 · 2018-08-17T18:12:56Z

@sandeep-krishnamurthy Ready for merge.

This reverts commit 633bef3.

* Adds Per G. da Silva to CONTRIBUTORS.md * Updates pick operation to also handle negative indices

perdasilva requested a review from anirudh2290 as a code owner August 8, 2018 16:38

perdasilva commented Aug 8, 2018

View reviewed changes

perdasilva force-pushed the feature/MXNET_791/pick_negative_indices branch 2 times, most recently from efb0c4f to 975c9db Compare August 8, 2018 17:49

haojin2 reviewed Aug 8, 2018

View reviewed changes

eric-haibin-lin added Operator pr-awaiting-response PR is reviewed and waiting for contributor to respond labels Aug 9, 2018

perdasilva force-pushed the feature/MXNET_791/pick_negative_indices branch 5 times, most recently from 0a4064b to a9fb7c2 Compare August 9, 2018 14:43

haojin2 reviewed Aug 9, 2018

View reviewed changes

perdasilva force-pushed the feature/MXNET_791/pick_negative_indices branch from a9fb7c2 to 0326116 Compare August 9, 2018 20:42

perdasilva force-pushed the feature/MXNET_791/pick_negative_indices branch from 0326116 to 8b23e19 Compare August 10, 2018 08:35

perdasilva force-pushed the feature/MXNET_791/pick_negative_indices branch from 8b23e19 to 21631b6 Compare August 11, 2018 10:25

perdasilva changed the title ~~[MXNET-791][WIP] Pick with negative indices~~ [MXNET-791] Pick with negative indices Aug 11, 2018

perdasilva force-pushed the feature/MXNET_791/pick_negative_indices branch from 21631b6 to eb6baf0 Compare August 13, 2018 08:43

perdasilva force-pushed the feature/MXNET_791/pick_negative_indices branch 2 times, most recently from be8c120 to ecd90aa Compare August 15, 2018 08:34

sandeep-krishnamurthy reviewed Aug 15, 2018

View reviewed changes

perdasilva force-pushed the feature/MXNET_791/pick_negative_indices branch 2 times, most recently from c106ef2 to 05e2915 Compare August 16, 2018 06:17

perdasilva added 2 commits August 16, 2018 15:58

Adds Per G. da Silva to CONTRIBUTORS.md

fdf3d6b

Updates pick operation to also handle negative indices

63bda11

perdasilva force-pushed the feature/MXNET_791/pick_negative_indices branch from 05e2915 to 63bda11 Compare August 16, 2018 13:59

sandeep-krishnamurthy approved these changes Aug 17, 2018

View reviewed changes

haojin2 approved these changes Aug 17, 2018

View reviewed changes

sandeep-krishnamurthy merged commit 633bef3 into apache:master Aug 17, 2018

marcoabreu added a commit that referenced this pull request Aug 17, 2018

Revert "[MXNET-791] Pick with negative indices (#12090)"

1dea56b

This reverts commit 633bef3.

piyushghai added a commit to piyushghai/incubator-mxnet that referenced this pull request Aug 17, 2018

Revert "[MXNET-791] Pick with negative indices (apache#12090)"

d818f66

This reverts commit 633bef3.

XinYao1994 pushed a commit to XinYao1994/incubator-mxnet that referenced this pull request Aug 29, 2018

[MXNET-791] Pick with negative indices (apache#12090)

3bf9eaf

* Adds Per G. da Silva to CONTRIBUTORS.md * Updates pick operation to also handle negative indices

zhreshold mentioned this pull request Jan 4, 2019

F.pick operator don't support negative indices and silently clip out of range indices #12009

Closed

perdasilva deleted the feature/MXNET_791/pick_negative_indices branch February 8, 2019 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MXNET-791] Pick with negative indices #12090

[MXNET-791] Pick with negative indices #12090

perdasilva commented Aug 8, 2018 •

edited

Loading

perdasilva commented Aug 8, 2018

perdasilva Aug 8, 2018

haojin2 commented Aug 8, 2018

haojin2 Aug 8, 2018

haojin2 Aug 8, 2018

perdasilva commented Aug 9, 2018

haojin2 Aug 9, 2018

haojin2 Aug 9, 2018

haojin2 Aug 9, 2018

haojin2 Aug 9, 2018

perdasilva commented Aug 9, 2018

perdasilva commented Aug 10, 2018

haojin2 commented Aug 10, 2018

perdasilva commented Aug 10, 2018 •

edited

Loading

haojin2 commented Aug 10, 2018

perdasilva commented Aug 11, 2018

perdasilva commented Aug 14, 2018

sandeep-krishnamurthy left a comment

sandeep-krishnamurthy Aug 15, 2018

perdasilva Aug 16, 2018

haojin2 commented Aug 15, 2018

perdasilva commented Aug 16, 2018

perdasilva commented Aug 16, 2018

perdasilva commented Aug 16, 2018

sandeep-krishnamurthy left a comment

sandeep-krishnamurthy commented Aug 17, 2018

haojin2 commented Aug 17, 2018

[MXNET-791] Pick with negative indices #12090

[MXNET-791] Pick with negative indices #12090

Conversation

perdasilva commented Aug 8, 2018 • edited Loading

Description

Checklist

Essentials

Changes

Comments

perdasilva commented Aug 8, 2018

Choose a reason for hiding this comment

haojin2 commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perdasilva commented Aug 9, 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

perdasilva commented Aug 9, 2018

perdasilva commented Aug 10, 2018

haojin2 commented Aug 10, 2018

perdasilva commented Aug 10, 2018 • edited Loading

haojin2 commented Aug 10, 2018

perdasilva commented Aug 11, 2018

perdasilva commented Aug 14, 2018

sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haojin2 commented Aug 15, 2018

perdasilva commented Aug 16, 2018

perdasilva commented Aug 16, 2018

perdasilva commented Aug 16, 2018

sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

sandeep-krishnamurthy commented Aug 17, 2018

haojin2 commented Aug 17, 2018

perdasilva commented Aug 8, 2018 •

edited

Loading

perdasilva commented Aug 10, 2018 •

edited

Loading