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

randn operator for symbol and NDarray API #12775

Closed
wants to merge 11 commits into from

Conversation

ChaiBapchya
Copy link
Contributor

@ChaiBapchya ChaiBapchya commented Oct 10, 2018

Description

Added the Missing randn operator for symbol API.
Fixes inconsistent function signature of ndarray.randn
Fixes #12755 and #12801

@piyushghai
Copy link
Contributor

@ChaiBapchya Can you check the lint failures on CI ?

@sandeep-krishnamurthy
Copy link
Contributor

@ChaiBapchya - Thanks for your contributions. Please add more detailed description from the template.
Why is it not named parameters? Why picking from kwargs? This might not be a good user experience for using the API. Thoughts?

@sandeep-krishnamurthy sandeep-krishnamurthy added Symbol pr-work-in-progress PR is still work in progress labels Oct 10, 2018
@sandeep-krishnamurthy
Copy link
Contributor

@anirudhacharya - Can you please take a look at this PR?

Copy link
Member

@anirudhacharya anirudhacharya left a comment

Choose a reason for hiding this comment

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

Yes I agree with @sandeep-krishnamurthy . the method definition is inconsistent with other similar sampling functions of the symbol API( line 74 in the same file, for example). Also please add tests for this.

@anirudhacharya
Copy link
Member

anirudhacharya commented Oct 10, 2018

Also if you are adding this operator in the symbol API, can you implement the operator along with its gradient in C++layer rather than implementing it only in python.

@ChaiBapchya ChaiBapchya changed the title Added randn operator for symbol API randn operator for symbol and NDarray API Oct 18, 2018
@ankkhedia
Copy link
Contributor

@ChaiBapchya ping!
Could you please take a look into the failing CI?

@@ -152,7 +152,7 @@ def normal(loc=0, scale=1, shape=_Null, dtype=_Null, ctx=None, out=None, **kwarg
[loc, scale], shape, dtype, ctx, out, kwargs)


def randn(*shape, **kwargs):
def randn(loc=0, scale=1, shape=_Null, dtype=_Null, ctx=None, out=None, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be a breaking change: For example mx.nd.random.randn(2, 3, loc=5, scale=1) will fail now. We should add this to APIs good to break for 2.0 #9686 and postpone this.

@ChaiBapchya
Copy link
Contributor Author

Closing, will reopen for 2.0

@ChaiBapchya ChaiBapchya deleted the randn branch November 13, 2018 21:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-work-in-progress PR is still work in progress Symbol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants