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

Add default parameters for Scala NDArray.arange #13816

Merged
merged 8 commits into from
Mar 6, 2019
Merged

Add default parameters for Scala NDArray.arange #13816

merged 8 commits into from
Mar 6, 2019

Conversation

kiendang
Copy link
Contributor

@kiendang kiendang commented Jan 9, 2019 •
edited
Loading

Description

Currently NDArray.arange requires manually passing all parameters.

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

Changes

Put back default parameters for NDArray.arange and update unit test.

Comments

Probably due to #12064 where a lot of changes were made and at the end default parameters got omitted unintentionally, which wasn't caught by tests.

@kiendang kiendang requested review from nswamy and yzhliu as code owners January 9, 2019 16:58
@marcoabreu marcoabreu added pr-awaiting-review PR is waiting for code review Scala labels Jan 9, 2019
@sandeep-krishnamurthy
Copy link
Contributor

@lanking520 @piyushghai - Can you please help review this PR?
@kiendang - Thanks for your contributions.

@piyushghai
Copy link
Contributor

@kiendang Thanks for your contributions.
The CI seems to have failed due to an unrelated reason.

Can you push an empty commit to re-trigger the CI ?

git commit -m --allow-empty "empty commit"

lanking520
lanking520 suggested changes Jan 28, 2019
View reviewed changes
scala-package/core/src/main/scala/org/apache/mxnet/NDArray.scala
* @param ctx Device context. Default context is the current default context.
* @param dType The data type of the `NDArray`. The default datatype is `DType.Float32`.
* @return NDArray of evenly spaced values in the specified range.
*/
def arange(start: Float, stop: Option[Float], step: Float,
repeat: Int, ctx: Context, dType: DType): NDArray = {
def arange(start: Float, stop: Option[Float] = None, step: Float = 1.0f,
Copy link
Member

Choose a reason for hiding this comment

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

Unless you are certain that this param is not going to be used, please place it at the end of the param list and rename it as inferRange follows Java camel case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not being used now. Do you mean we still include it now in case it'll be used later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to #12064 (comment) infer_range is only applicable to symbolic API and was meant to be removed from ndarray API.

Copy link
Member

Choose a reason for hiding this comment

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

Add @taliesinb here to bring more context since I am not very familiar with it

@kiendang kiendang requested a review from szha as a code owner January 29, 2019 09:40
lanking520
lanking520 previously approved these changes Jan 29, 2019
View reviewed changes
@szha szha added the Breaking label Jan 30, 2019
szha
szha suggested changes Jan 30, 2019
View reviewed changes
Copy link
Member

@szha szha left a comment

Choose a reason for hiding this comment

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

The infer_range change breaks python API.

@lanking520 lanking520 dismissed their stale review January 30, 2019 01:19

Not responsible for this code base

@kiendang
Copy link
Contributor Author

@szha From my understanding infer_range was proposed in #12064 to infer stop from output shape, but later in the same PR deemed only applicable to symbolic and not ndarray API and was meant to be ripped out from all languages' ndarray API. This was done for Scala and Clojure and I believe should have been done for python as well.

@taliesinb could you verify?

@szha
Copy link
Member

szha commented Jan 31, 2019

@kiendang since the python interface was already released in previous versions, any code that includes value for infer_range in the arange python will be broken because of the removal.

@kiendang
Copy link
Contributor Author

I doubt anyone would manually include infer_range since setting non-default value infer_range=True for ndarray.arange always returns error. None of the examples seem to use it either.

Still I get your point. I will revert the change if you think it's best to do so. If noone uses infer_range then leaving it there doesn't hurt. However, if infer_range is pointless and was not taken out by mistake, what's the best way to deal with that?

@szha
Copy link
Member

szha commented Jan 31, 2019 •
edited
Loading

Thanks for the explanation. Since this option never worked, I'd recommend setting the python default value for it to None, and having a warning for the option indicating that this option is deprecated and ignored if it gets set to True or False (or not None in general).

@kiendang
Copy link
Contributor Author

Hmm tests finished and passed but status wasn't updated here.

@szha
Copy link
Member

szha commented Jan 31, 2019

@kiendang this is a known issue and @marcoabreu is working on it

@vandanavk
Copy link
Contributor

What are the next steps on this PR?

@kiendang
Copy link
Contributor Author

kiendang commented Feb 6, 2019

Nothing more to add from me.

@ankkhedia
Copy link
Contributor

@szha Could you please look into the updated PR and review/merge?

@kiendang
Copy link
Contributor Author

kiendang commented Mar 4, 2019

@szha May I know what's the holdup? I use arange from time to time and it's a bit annoying currently.

@szha szha merged commit a0f3f92 into apache:master Mar 6, 2019
@kiendang kiendang deleted the fix-arange-scala branch March 8, 2019 02:11
@andrewfayres andrewfayres mentioned this pull request Mar 12, 2019
Closed
vdantu pushed a commit to vdantu/incubator-mxnet that referenced this pull request Mar 31, 2019
@kiendang @vdantu
Add default parameters for Scala NDArray.arange (apache#13816)
777d678 
* Add default arguments for arange

* Remove redundant tag

* Update test

* Remove infer_range for python ndarray.arange

* Update CONTRIBUTORS.md

* Deprecate infer_range argument in ndarray.arange
nswamy pushed a commit that referenced this pull request Apr 5, 2019
@kiendang @nswamy
Add default parameters for Scala NDArray.arange (#13816)
f6dc492 
* Add default arguments for arange

* Remove redundant tag

* Update test

* Remove infer_range for python ndarray.arange

* Update CONTRIBUTORS.md

* Deprecate infer_range argument in ndarray.arange
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@kiendang
Add default parameters for Scala NDArray.arange (apache#13816)
e8abcfe 
* Add default arguments for arange

* Remove redundant tag

* Update test

* Remove infer_range for python ndarray.arange

* Update CONTRIBUTORS.md

* Deprecate infer_range argument in ndarray.arange
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lanking520 lanking520 lanking520 left review comments

@szha szha szha approved these changes

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

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

Assignees
No one assigned
Labels
Breaking pr-awaiting-review PR is waiting for code review Scala
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@kiendang @sandeep-krishnamurthy @piyushghai @szha @vandanavk @ankkhedia @lanking520 @marcoabreu