Skip to content

[SPARK-20501] [ML] ML 2.2 QA: New Scala APIs, docs #17934

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 4 commits into from
Closed

[SPARK-20501] [ML] ML 2.2 QA: New Scala APIs, docs #17934

yanboliang wants to merge 4 commits into from

Conversation

@yanboliang
Copy link
Contributor

@yanboliang yanboliang commented May 10, 2017

What changes were proposed in this pull request?

Review new Scala APIs introduced in 2.2.

How was this patch tested?

Existing tests.

@SparkQA
Copy link

SparkQA commented May 10, 2017

Test build #76742 has finished for PR 17934 at commit 427295e.

  • This patch fails to generate documentation.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class Imputer @Since(\"2.2.0\")(@Since(\"2.2.0\") override val uid: String)

MLnick
MLnick reviewed May 10, 2017
View reviewed changes
mllib/src/main/scala/org/apache/spark/ml/classification/LinearSVC.scala
*/
@Since("2.2.0")
def setThreshold(value: Double): this.type = set(threshold, value)
setDefault(threshold -> 0.0)
Copy link
Contributor

@MLnick MLnick May 10, 2017

Choose a reason for hiding this comment

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

why remove this?

Copy link
Contributor Author

@yanboliang yanboliang May 10, 2017

Choose a reason for hiding this comment

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

Typo, reverted, thanks.

MLnick
MLnick reviewed May 10, 2017
View reviewed changes
mllib/src/main/scala/org/apache/spark/ml/feature/StringIndexer.scala Outdated
*/
@Since("1.4.0")
class StringIndexerModel (
class StringIndexerModel private[ml] (
Copy link
Contributor

@MLnick MLnick May 10, 2017

Choose a reason for hiding this comment

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

Hmm, making this private now will break user code if it is used, no? It's unfortunate that it wasn't private before, but do we want to do this?

Copy link
Contributor Author

@yanboliang yanboliang May 10, 2017

Choose a reason for hiding this comment

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

We make this public by mistake, so I think we should fix this bug. AFAIK, this is the only model whose constructor is public, so there is little chance to break user code. And I think it makes sense to break user code by fixing bug. Thanks.

Copy link
Member

@jkbradley jkbradley May 16, 2017

Choose a reason for hiding this comment

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

I thought this was made public on purpose. StringIndexerModel is one case where it makes sense for users to create it manually (if they already have a list of the possible Strings).

Copy link
Contributor Author

@yanboliang yanboliang May 16, 2017

Choose a reason for hiding this comment

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

OK, I reverted back. Thanks.

MLnick
MLnick reviewed May 10, 2017
View reviewed changes
mllib/src/main/scala/org/apache/spark/ml/feature/Imputer.scala Outdated
*/
@Experimental
@Since("2.2.0")
class ImputerModel private[ml](
Copy link
Contributor

@MLnick MLnick May 10, 2017

Choose a reason for hiding this comment

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

nit: should be a space between private[ml] and the (

MLnick
MLnick reviewed May 10, 2017
View reviewed changes
Copy link
Contributor

@MLnick MLnick left a comment

Choose a reason for hiding this comment

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

Thanks for this and picking up the oversights on since tags for Imputer.

I don't think we should be changing the threshold for SVC? It should be 0 no? cc @hhbyyh

@SparkQA
Copy link

SparkQA commented May 10, 2017

Test build #76746 has finished for PR 17934 at commit 2aab9ba.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class Imputer @Since(\"2.2.0\") (@Since(\"2.2.0\") override val uid: String)

@yanboliang
Copy link
Contributor Author

yanboliang commented May 10, 2017

@MLnick Thanks for your kindly remind, the change for LinearSVC threshold is typo, I reverted.

@SparkQA
Copy link

SparkQA commented May 10, 2017

Test build #76751 has finished for PR 17934 at commit 65f22a0.

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

felixcheung
felixcheung reviewed May 11, 2017
View reviewed changes
mllib/src/main/scala/org/apache/spark/ml/classification/LinearSVC.scala
* Linear SVM Classifier</a>
*
* This binary classifier optimizes the Hinge Loss using the OWLQN optimizer.
* Only supports L2 regularization currently.
Copy link
Member

@felixcheung felixcheung May 11, 2017

Choose a reason for hiding this comment

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

is this something should be mentioned in R too?

Copy link
Contributor Author

@yanboliang yanboliang May 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@felixcheung Yeah, it should be. However, since SparkR exposed more MLlib APIs during 2.2 releasing cycle, I'm preparing a separate PR to audit new SparkR MLlib APIs which would include this update. Thanks.

@SparkQA
Copy link

SparkQA commented May 16, 2017

Test build #76956 has finished for PR 17934 at commit cef372b.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class StringIndexerModel (

@jkbradley
Copy link
Member

jkbradley commented May 16, 2017

LGTM
I'll merge this with master and branch-2.2
Thanks all!

asfgit pushed a commit that referenced this pull request May 16, 2017
@yanboliang @jkbradley
[SPARK-20501][ML] ML 2.2 QA: New Scala APIs, docs
57c87cf 
## What changes were proposed in this pull request?
Review new Scala APIs introduced in 2.2.

## How was this patch tested?
Existing tests.

Author: Yanbo Liang <ybliang8@gmail.com>

Closes #17934 from yanboliang/spark-20501.

(cherry picked from commit dbe8163)
Signed-off-by: Joseph K. Bradley <joseph@databricks.com>
@asfgit asfgit closed this in dbe8163 May 16, 2017
@yanboliang yanboliang deleted the spark-20501 branch May 16, 2017 04:55
robert3005 pushed a commit to palantir/spark that referenced this pull request May 19, 2017
@yanboliang
[SPARK-20501][ML] ML 2.2 QA: New Scala APIs, docs
80fa452 
## What changes were proposed in this pull request?
Review new Scala APIs introduced in 2.2.

## How was this patch tested?
Existing tests.

Author: Yanbo Liang <ybliang8@gmail.com>

Closes apache#17934 from yanboliang/spark-20501.
lycplus pushed a commit to lycplus/spark that referenced this pull request May 24, 2017
@yanboliang @lycplus
[SPARK-20501][ML] ML 2.2 QA: New Scala APIs, docs
d649dfe 
## What changes were proposed in this pull request?
Review new Scala APIs introduced in 2.2.

## How was this patch tested?
Existing tests.

Author: Yanbo Liang <ybliang8@gmail.com>

Closes apache#17934 from yanboliang/spark-20501.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixcheung felixcheung felixcheung left review comments

+2 more reviewers

@MLnick MLnick MLnick left review comments

@jkbradley jkbradley jkbradley left review comments

Reviewers whose approvals may not affect merge requirements

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 @jkbradley @MLnick @felixcheung