Skip to content

[SPARK-23107][ML] ML 2.3 QA: New Scala APIs, docs. #20459

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
yanboliang wants to merge 1 commit into from
Closed

[SPARK-23107][ML] ML 2.3 QA: New Scala APIs, docs. #20459

yanboliang wants to merge 1 commit into from

Conversation

@yanboliang
Copy link
Contributor

@yanboliang yanboliang commented Jan 31, 2018

What changes were proposed in this pull request?

Audit new APIs and docs in 2.3.0.

How was this patch tested?

No test.

@SparkQA
Copy link

SparkQA commented Jan 31, 2018

Test build #86889 has finished for PR 20459 at commit c24cc14.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

yanboliang
yanboliang commented Jan 31, 2018
View reviewed changes
mllib/src/main/scala/org/apache/spark/ml/feature/QuantileDiscretizer.scala Outdated
Copy link
Contributor Author

@yanboliang yanboliang Jan 31, 2018

Choose a reason for hiding this comment

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

For this and the followings who added before 2.2, this involves breaking change in a way, but I think we should keep them final to prevent being changed(as we did for other param variables). cc @MLnick @WeichenXu123

Copy link
Contributor

@WeichenXu123 WeichenXu123 Feb 1, 2018

Choose a reason for hiding this comment

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

I'm more prefer to mark them as non final, since if user want to extend these estimators they would be able to override handleInvalid to define their own handleInvalid valid values (through doc parameter of Param class constructor) , this is different from other classes. What do you think of it?

Copy link
Contributor Author

@yanboliang yanboliang Feb 1, 2018

Choose a reason for hiding this comment

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

Fair enough. I will leave handleInvalid in all estimators non-final.

@SparkQA
Copy link

SparkQA commented Feb 1, 2018

Test build #86891 has finished for PR 20459 at commit 407cd80.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Feb 1, 2018

Test build #86903 has finished for PR 20459 at commit b8c6c19.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@WeichenXu123 WeichenXu123 mentioned this pull request Feb 1, 2018
Closed
asfgit pushed a commit that referenced this pull request Feb 1, 2018
@yanboliang
[SPARK-23107][ML] ML 2.3 QA: New Scala APIs, docs.
205bce9 
## What changes were proposed in this pull request?
Audit new APIs and docs in 2.3.0.

## How was this patch tested?
No test.

Author: Yanbo Liang <[email protected]>

Closes #20459 from yanboliang/SPARK-23107.

(cherry picked from commit e15da5b)
Signed-off-by: Nick Pentreath <[email protected]>
@MLnick
Copy link
Contributor

MLnick commented Feb 1, 2018

Merged to master / branch-2.3. Thanks @yanboliang !

@asfgit asfgit closed this in e15da5b Feb 1, 2018
@yanboliang yanboliang deleted the SPARK-23107 branch February 1, 2018 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixcheung felixcheung felixcheung approved these changes

@WeichenXu123 WeichenXu123 WeichenXu123 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yanboliang @SparkQA @MLnick @felixcheung @WeichenXu123