[GitHub] [spark] zhengruifeng commented on a change in pull request #28884: [SPARK-20249][ML][PYSPARK] Add training summary for LinearSVCModel

[GitBox <gi...@apache.org>]

zhengruifeng commented on a change in pull request #28884:
URL: https://github.com/apache/spark/pull/28884#discussion_r443924069



##########
File path: mllib/src/main/scala/org/apache/spark/ml/classification/LinearSVC.scala
##########
@@ -439,3 +502,55 @@ object LinearSVCModel extends MLReadable[LinearSVCModel] {
     }
   }
 }
+
+/**
+ * Abstraction for LinearSVC results for a given model.
+ */
+sealed trait LinearSVCSummary extends BinaryClassificationSummary {

Review comment:
       nit: uncessary braces

##########
File path: mllib/src/main/scala/org/apache/spark/ml/classification/LinearSVC.scala
##########
@@ -439,3 +502,55 @@ object LinearSVCModel extends MLReadable[LinearSVCModel] {
     }
   }
 }
+
+/**
+ * Abstraction for LinearSVC results for a given model.
+ */
+sealed trait LinearSVCSummary extends BinaryClassificationSummary {
+}
+
+/**
+ * Abstraction for LinearSVC training results.
+ */
+sealed trait LinearSVCTrainingSummary extends LinearSVCSummary with TrainingSummary {
+}

Review comment:
       ditto

##########
File path: mllib/src/main/scala/org/apache/spark/ml/classification/LinearSVC.scala
##########
@@ -368,6 +388,49 @@ class LinearSVCModel private[classification] (
     BLAS.dot(features, coefficients) + intercept
   }
 
+  /**
+   * Gets summary of model on training set. An exception is thrown
+   * if `hasSummary` is false.
+   */
+  @Since("3.1.0")
+  override def summary: LinearSVCTrainingSummary = super.summary
+
+  /**
+   * If the rawPrediction and prediction columns are set, this method returns the current model,
+   * otherwise it generates new columns for them and sets them as columns on a new copy of
+   * the current model
+   */
+  private[classification] def findSummaryModel():
+  (LinearSVCModel, String, String) = {

Review comment:
       nit:
   `  private[classification] def findSummaryModel(): (LinearSVCModel, String, String) = {` ?

##########
File path: mllib/src/main/scala/org/apache/spark/ml/classification/LinearSVC.scala
##########
@@ -267,7 +267,27 @@ class LinearSVC @Since("2.2.0") (
       if (featuresStd(i) != 0.0) rawCoefficients(i) / featuresStd(i) else 0.0
     }
     val intercept = if ($(fitIntercept)) rawCoefficients.last else 0.0
-    copyValues(new LinearSVCModel(uid, Vectors.dense(coefficientArray), intercept))
+    val vec = Vectors.dense(coefficientArray)
+    return createModel(dataset, vec, intercept, objectiveHistory)

Review comment:
       I'd like: `createModel(dataset, Vectors.dense(coefficientArray), intercept, objectiveHistory)`
   var `vec` and `return` are unnecessary




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org