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

Add symbol api for randn and fix shape issue for randn ndarray and symbol api#15772

Merged
roywei merged 11 commits intoapache:masterfrom
ChaiBapchya:randn_symbol
Aug 25, 2019
Merged

Add symbol api for randn and fix shape issue for randn ndarray and symbol api#15772
roywei merged 11 commits intoapache:masterfrom
ChaiBapchya:randn_symbol

Conversation

@ChaiBapchya
Copy link
Contributor

@ChaiBapchya ChaiBapchya commented Aug 7, 2019 •
edited
Loading

Description

  1. Randn is not currently present in symbol API
    Fixes Absence of randn function in Symbol API #12755

  2. Currently randn gives issue when passed NDArray to the randn function parameters loc and scale

>>> mx.nd.random.randn((2, 3), loc=a, scale=a)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/bapac/workspace/incubator-mxnet/python/mxnet/ndarray/random.py", line 223, in randn
    assert isinstance(loc, (int, float))
AssertionError

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:
  • Code is well-documented:
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Original works as well
loc and scale scalar

>>> mx.nd.random.randn(2, 3, loc=5, scale=1)

[[7.2122064 5.774004  6.0434403]
 [6.1839256 6.891711  3.7652586]]
<NDArray 2x3 @cpu(0)>

loc and scale as NDArray

>>> a=mx.nd.array([[1,2],[3,4]])
>>> a

[[1. 2.]
 [3. 4.]]
<NDArray 2x2 @cpu(0)>
>>> mx.nd.random.randn(2, 3, loc=a, scale=a)

[[[[-0.771029    0.5486156   1.5793836 ]
   [-0.85608196 -0.9768796   0.7919808 ]]

  [[ 2.4888437   1.9256786   1.0245001 ]
   [ 1.9547654   3.1492283   4.9322524 ]]]


 [[[ 5.0588713   4.064883    6.2195086 ]
   [ 3.3605237   0.08666945  0.6729102 ]]

  [[ 0.8471296   6.967091   -1.8937755 ]
   [-0.29237127 -0.16993093 -1.3115396 ]]]]
<NDArray 2x2x2x3 @cpu(0)>

@ChaiBapchya ChaiBapchya requested a review from szha as a code owner August 7, 2019 02:02
@ChaiBapchya
Copy link
Contributor Author

ChaiBapchya commented Aug 7, 2019

@mxnet-label-bot add [Bug, Operator, Doc]

@access2rohit
Copy link
Contributor

access2rohit commented Aug 7, 2019

Are you sure that its for Symbolic as well ? If thats not a restriction shouldn't we add unittest for symbolic mode too ?

kshitij12345
kshitij12345 reviewed Aug 7, 2019
View reviewed changes
@ChaiBapchya
Copy link
Contributor Author

ChaiBapchya commented Aug 7, 2019

Addressed comment. Verified unit-tests work with

$ nosetests tests/python/unittest/test_random.py
......
----------------------------------------------------------------------
Ran 23 tests in 2158.096s

OK

For these build flags
[✖ CUDA, ✖ CUDNN, ✖ NCCL, ✖ CUDA_RTC, ✖ TENSORRT, ✔ CPU_SSE, ✔ CPU_SSE2, ✔ CPU_SSE3, ✔ CPU_SSE4_1, ✔ CPU_SSE4_2, ✖ CPU_SSE4A, ✔ CPU_AVX, ✖ CPU_AVX2, ✔ OPENMP, ✖ SSE, ✔ F16C, ✔ JEMALLOC, ✔ BLAS_OPEN, ✖ BLAS_ATLAS, ✖ BLAS_MKL, ✖ BLAS_APPLE, ✔ LAPACK, ✔ MKLDNN, ✔ OPENCV, ✖ CAFFE, ✖ PROFILER, ✖ DIST_KVSTORE, ✖ CXX14, ✖ INT64_TENSOR_SIZE, ✔ SIGNAL_HANDLER, ✔ DEBUG, ✖ TVM_OP]

kshitij12345
kshitij12345 reviewed Aug 7, 2019
View reviewed changes
apeforest
apeforest reviewed Aug 7, 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. Please add unit tests as @kshitij12345 suggested.

kshitij12345
kshitij12345 reviewed Aug 7, 2019
View reviewed changes
kshitij12345
kshitij12345 reviewed Aug 7, 2019
View reviewed changes
@access2rohit
Copy link
Contributor

access2rohit commented Aug 7, 2019

LGTM

@ChaiBapchya ChaiBapchya mentioned this pull request Aug 7, 2019
Open
@roywei
Copy link
Member

roywei commented Aug 19, 2019

@ChaiBapchya gentle ping to trigger CI so we can merge. thanks!

@ChaiBapchya
Copy link
Contributor Author

ChaiBapchya commented Aug 19, 2019

@apeforest @kshitij12345 addressed! Pl review. Thank you!

kshitij12345
kshitij12345 reviewed Aug 20, 2019
View reviewed changes
@ChaiBapchya
Copy link
Contributor Author

ChaiBapchya commented Aug 24, 2019

@kshitij12345 @apeforest @roywei ready for review.

@roywei roywei merged commit 2e4242e into apache:master Aug 25, 2019
@ChaiBapchya ChaiBapchya deleted the randn_symbol branch August 25, 2019 22:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@szha szha Awaiting requested review from szha szha is a code owner

3 more reviewers

@apeforest apeforest apeforest left review comments

@kshitij12345 kshitij12345 kshitij12345 left review comments

@roywei roywei roywei approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug Doc Operator

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Absence of randn function in Symbol API

6 participants

@ChaiBapchya @access2rohit @roywei @apeforest @kshitij12345 @marcoabreu