Skip to content

[SPARK-13962][ML] spark.ml Evaluators should support other numeric types for label#12500

Closed
BenFradet wants to merge 8 commits intoapache:masterfrom
BenFradet:SPARK-13962
Closed

[SPARK-13962][ML] spark.ml Evaluators should support other numeric types for label#12500
BenFradet wants to merge 8 commits intoapache:masterfrom
BenFradet:SPARK-13962

Conversation

@BenFradet
Copy link
Contributor

What changes were proposed in this pull request?

Made BinaryClassificationEvaluator, MulticlassClassificationEvaluator and RegressionEvaluator accept all numeric types for label

How was this patch tested?

Unit tests

@SparkQA
Copy link

SparkQA commented Apr 19, 2016

Test build #56235 has finished for PR 12500 at commit 24e45e3.

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

@SparkQA
Copy link

SparkQA commented Apr 19, 2016

Test build #56259 has finished for PR 12500 at commit 2bec69e.

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

@BenFradet
Copy link
Contributor Author

pinging @MLnick

(prediction, label)
.rdd
.map {
case Row(prediction: Double, label: Double) => (prediction, label)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor but can this fit on one line? or have .map { case Row ... }

@MLnick
Copy link
Contributor

MLnick commented Apr 21, 2016

LGTM. Will leave open a little while in case anyone else wants to take a look. @sethah?

* @return DataFrame with metadata
*/
def setMetadata(data: DataFrame, numClasses: Int, labelColName: String): DataFrame = {
def setMetadata(data: DataFrame,
Copy link
Contributor

Choose a reason for hiding this comment

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

Follow Spark style here:

def setMetadata(
      data: DataFrame,
      numClasses: Int,
      labelColName: String,
      featuresColName: String): DataFrame = {

@BenFradet
Copy link
Contributor Author

@MLnick @sethah thanks for the reviews, will fix.

@sethah
Copy link
Contributor

sethah commented Apr 21, 2016

A couple minor syntax comments, other than that LGTM.

@SparkQA
Copy link

SparkQA commented Apr 21, 2016

Test build #56553 has finished for PR 12500 at commit ec54d74.

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

val thrown = intercept[IllegalArgumentException] {
evaluator.evaluate(dfWithStringLabels)
}
assert(thrown.getMessage contains
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor style issue, but Spark code style prefers not to use infix notation (see https://cwiki.apache.org/confluence/display/SPARK/Spark+Code+Style+Guide). Could you do thrown.getMessage.contains(...) instead? And also change the occurrence above in L57.

@MLnick
Copy link
Contributor

MLnick commented Apr 25, 2016

@BenFradet just one more minor style comment, then I think this is ready to merge.

@BenFradet
Copy link
Contributor Author

@MLnick will do

@SparkQA
Copy link

SparkQA commented Apr 25, 2016

Test build #56916 has finished for PR 12500 at commit 6c66068.

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

@MLnick
Copy link
Contributor

MLnick commented Apr 26, 2016

@BenFradet Thanks! Merged to master. Thanks @sethah for the review.

@asfgit asfgit closed this in 2a5c930 Apr 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants