Skip to content
This repository has been 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
merged 11 commits into from
Aug 25, 2019

Conversation

ChaiBapchya
Copy link
Contributor

@ChaiBapchya ChaiBapchya commented Aug 7, 2019

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

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

@access2rohit
Copy link
Contributor

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

@ChaiBapchya
Copy link
Contributor Author

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]

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.

@access2rohit
Copy link
Contributor

LGTM

@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

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

@ChaiBapchya
Copy link
Contributor Author

@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Absence of randn function in Symbol API
6 participants