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

[MXNET-1029] Feature request: randint operator #12749

Merged
merged 35 commits into from
Nov 27, 2018

Conversation

ChaiBapchya
Copy link
Contributor

@ChaiBapchya ChaiBapchya commented Oct 6, 2018

Description

Incorporates the feature request to support randint operator, includes optional tag across all random functions.
Fixes #11218

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

Comments

  • Flakiness of test_randint_generator
    This function tries to ensure the discrete uniform distribution is truly uniform.
    It uses the already defined verify_generator function but with default values, it failed the CI for certain config (specifically Python2 GPU.
    Ensured that I check in the above environment and verified the function works as mentioned in feedback.
MXNET_TEST_COUNT=10000 python -m nose -v --nocapture incubator-mxnet/tests/python/unittest/test_random.py:test_randint_generator --logging-level=DEBUG

@vrakesh
Copy link
Contributor

vrakesh commented Oct 7, 2018

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

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Oct 7, 2018
tests/python/unittest/test_operator.py Outdated Show resolved Hide resolved
python/mxnet/ndarray/random.py Show resolved Hide resolved
tests/python/unittest/test_operator.py Outdated Show resolved Hide resolved
python/mxnet/ndarray/random.py Outdated Show resolved Hide resolved
src/operator/random/sample_op.cc Show resolved Hide resolved
python/mxnet/ndarray/random.py Outdated Show resolved Hide resolved
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.

Please create unit test (refer to test_random.py). Also please explicitly check edge cases like randint(dtype='int64', low=50000000, high=50000010)

@sandeep-krishnamurthy sandeep-krishnamurthy added pr-work-in-progress PR is still work in progress Operator and removed pr-awaiting-review PR is waiting for code review labels Oct 9, 2018
@ChaiBapchya ChaiBapchya reopened this Oct 11, 2018
tests/python/unittest/test_random.py Outdated Show resolved Hide resolved
src/operator/random/sample_op.h Outdated Show resolved Hide resolved
src/operator/random/sample_op.h Outdated Show resolved Hide resolved
python/mxnet/ndarray/random.py Outdated Show resolved Hide resolved
@eric-haibin-lin eric-haibin-lin dismissed their stale review November 21, 2018 21:03

comments addressed. good work

@@ -63,6 +63,10 @@ class RandGenerator<cpu, DType> {

MSHADOW_XINLINE int rand() { return engine_->operator()(); }

MSHADOW_XINLINE int64_t rand_int64() {
return static_cast<int64_t>(engine_->operator()() << 31) + engine_->operator()();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@leezu over here I use rand_int64() by using mt19937.rand() for 2 32bits

According to the video : https://channel9.msdn.com/Events/GoingNative/2013/rand-Considered-Harmful
is this a right way of doing it truly uniform random integer distribution?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, because you're concatenating 2 32bit numbers, where every bit is uniformly random. Thus the resulting 64bits are uniformly random.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Operator pr-work-in-progress PR is still work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: randint operator
10 participants