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

Fix nd.pick large array issue #14082

Merged
merged 4 commits into from
Feb 16, 2019
Merged

Fix nd.pick large array issue #14082

merged 4 commits into from
Feb 16, 2019

Conversation

ChaiBapchya
Copy link
Contributor

Description

large op support (using index_t and size_t instead of int) based off of #13418
Fixes #9314

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • 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:https://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

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@ChaiBapchya
Copy link
Contributor Author

ChaiBapchya commented Feb 7, 2019 •
edited
Loading

@mxnet-label-bot add [pr-awaiting-review, NDArray]

@marcoabreu marcoabreu added pr-awaiting-review PR is waiting for code review NDArray labels Feb 7, 2019
eric-haibin-lin
eric-haibin-lin suggested changes Feb 11, 2019
View reviewed changes
Copy link
Member

@eric-haibin-lin eric-haibin-lin 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 fix! Could you add an unit test?

apeforest
apeforest suggested changes Feb 12, 2019
View reviewed changes
Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ChaiBapchya for your contribution! Overall looks good. Please add a test in test_large_array under nightly.

src/operator/tensor/broadcast_reduce_op.h Outdated
MSHADOW_XINLINE static void Map(int i, DType* out, const DType* a,
const IType *idx, int M, int stride,
MSHADOW_XINLINE static void Map(index_t i, DType* out, const DType* a,
const IType *idx, size_t M, int stride,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use index_t for M so you don't need so many casting in this function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for using index_t for M.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. i looked at other places where large array support was mentioned. It seemed that
index_t was used where the variable is used as index
and size_t was used otherwise. Hence I stuck to that. But I'll change it to avoid casting if that's what's useful.

access2rohit
access2rohit reviewed Feb 12, 2019
View reviewed changes
Copy link
Contributor

@access2rohit access2rohit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add test case to test for large array ?

@eric-haibin-lin
Copy link
Member

lgtm

apeforest
apeforest approved these changes Feb 16, 2019
View reviewed changes
Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@eric-haibin-lin eric-haibin-lin merged commit ed0b791 into apache:master Feb 16, 2019
@ChaiBapchya ChaiBapchya deleted the fix_nd_pick_large_array branch February 16, 2019 05:03
stephenrawls pushed a commit to stephenrawls/incubator-mxnet that referenced this pull request Feb 16, 2019
@ChaiBapchya @stephenrawls
Fix nd.pick large array issue (apache#14082)
d207d21 
* large op support

* replaced size_t with index_t for M, added test case

* changed shape
jessr92 pushed a commit to jessr92/incubator-mxnet that referenced this pull request Feb 19, 2019
@ChaiBapchya
Fix nd.pick large array issue (apache#14082)
ced59ae 
* large op support

* replaced size_t with index_t for M, added test case

* changed shape
drivanov pushed a commit to drivanov/incubator-mxnet that referenced this pull request Mar 4, 2019
@ChaiBapchya @drivanov
Fix nd.pick large array issue (apache#14082)
66a90ad 
* large op support

* replaced size_t with index_t for M, added test case

* changed shape
vdantu pushed a commit to vdantu/incubator-mxnet that referenced this pull request Mar 31, 2019
@ChaiBapchya @vdantu
Fix nd.pick large array issue (apache#14082)
e532d76 
* large op support

* replaced size_t with index_t for M, added test case

* changed shape
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@ChaiBapchya
Fix nd.pick large array issue (apache#14082)
07004a9 
* large op support

* replaced size_t with index_t for M, added test case

* changed shape
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@yuxihu yuxihu yuxihu left review comments

@access2rohit access2rohit access2rohit left review comments

@eric-haibin-lin eric-haibin-lin eric-haibin-lin approved these changes

@apeforest apeforest apeforest approved these changes

Assignees
No one assigned
Labels
NDArray 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.

nd.pick core dumps on large array
6 participants
@ChaiBapchya @eric-haibin-lin @yuxihu @apeforest @access2rohit @marcoabreu