[SPARK-21108] [ML] convert LinearSVC to aggregator framework #18315

hhbyyh · 2017-06-15T18:43:41Z

What changes were proposed in this pull request?

convert LinearSVC to new aggregator framework

How was this patch tested?

existing unit test.

hhbyyh · 2017-06-15T18:44:00Z

Will send further update after #18305 merged.

SparkQA · 2017-06-15T19:43:28Z

Test build #78118 has finished for PR 18315 at commit 071ce88.

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

sethah · 2017-06-16T20:18:17Z

Thanks for this pr @hhbyyh. I think we need to add a test suite for the aggregator, but since #18305 needs to be merged first, it's fine to wait. If you wouldn't mind helping to review #18305, that would be great :)

sethah · 2017-07-26T16:19:51Z

ping! #18305 was merged. This can proceed.

SparkQA · 2017-07-27T00:25:49Z

Test build #79981 has finished for PR 18315 at commit c8228fb.

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

MLnick · 2017-07-27T11:23:21Z

mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/HingeAggregatorSuite.scala

+    val interceptArray = Array(2.0)
+    val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+      fitIntercept = true)
+    withClue("LogisticAggregator does not support negative instance weights") {


nit: LogisticAggregator -> HingeAggregator

MLnick · 2017-07-27T11:28:33Z

mllib/src/main/scala/org/apache/spark/ml/optim/aggregator/HingeAggregator.scala

+ * "squared_hinge", as used in binary classification for instances in sparse or dense
+ * vector in an online fashion.
+ *
+ * Two LinearSVCAggregator can be merged together to have a summary of loss and gradient of


should be HingeAggregator here and elsewhere in this class

MLnick · 2017-07-27T11:32:29Z

Made a few minor comments, overall looks good.

MLnick · 2017-07-27T11:33:45Z

mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/HingeAggregatorSuite.scala

+    assert(loss ~== agg.loss relTol 0.01)
+    assert(gradient ~== agg.gradient relTol 0.01)
+  }
+


The other aggregator tests have one for "zero standard deviation". We should add one here too.

Sure. Added.

SparkQA · 2017-07-28T19:00:56Z

Test build #80028 has finished for PR 18315 at commit 94e0250.

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

hhbyyh · 2017-07-29T14:34:44Z

Thanks for the review. Updated to address the comments.

SparkQA · 2017-08-15T07:04:50Z

Test build #80661 has finished for PR 18315 at commit 94e0250.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

yanboliang · 2017-08-16T05:59:59Z

Jenkins, test this please.

yanboliang · 2017-08-16T06:01:36Z

@hhbyyh Would you mind to remove WIP in the PR title if it's applicable. I'll take a look soon. Thanks.

SparkQA · 2017-08-16T07:04:49Z

Test build #80719 has finished for PR 18315 at commit 94e0250.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

yanboliang · 2017-08-16T14:37:01Z

Jenkins, test this please.

SparkQA · 2017-08-16T15:43:52Z

Test build #80739 has finished for PR 18315 at commit 94e0250.

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

yanboliang

@hhbyyh Looks good overall, left some minor comments. Thanks.

yanboliang · 2017-08-17T09:19:16Z

mllib/src/test/scala/org/apache/spark/ml/classification/LinearSVCSuite.scala

@@ -173,7 +174,7 @@ class LinearSVCSuite extends SparkFunSuite with MLlibTestSparkContext with Defau
  test("sparse coefficients in SVCAggregator") {


SVCAggregator -> HingeAggregator?

yanboliang · 2017-08-17T09:26:56Z

mllib/src/main/scala/org/apache/spark/ml/optim/aggregator/HingeAggregator.scala

+
+/**
+ * HingeAggregator computes the gradient and loss for loss function ("hinge" or
+ * "squared_hinge", as used in binary classification for instances in sparse or dense


It seems this only support hinge loss currently. BTW, if we support squared hinge in the future, what is the best way? Add a param loss function for HingeAggregator or just add a new SquaredHingeAggregator? The later way should be more clear, but with more code.

I would prefer to use SquaredHingeAggregator. API-wise, it looks more intuitive and consistent to me. We can continue the review in the other LinearSVC PR.

Yeah, I agree you for separate SquaredHingeAggregator. Then we should remove squared_hinge from here?
BTW, you missed right parenthesis here.

yanboliang · 2017-08-17T09:32:25Z

mllib/src/main/scala/org/apache/spark/ml/classification/LinearSVC.scala

+      val regularization = if (regParamL2 != 0.0) {
+        val shouldApply = (idx: Int) => idx >= 0 && idx < numFeatures
+        Some(new L2Regularization(regParamL2, shouldApply,
+          if ($(standardization)) None else Some(featuresStd)))


Minor: The third argument applyFeaturesStd is a function rather than an array in semantics:

private[ml] class L2Regularization( override val regParam: Double, shouldApply: Int => Boolean, applyFeaturesStd: Option[Int => Double]) extends DifferentiableRegularization[Vector]

In LiR and LoR, we use a function:

val getFeaturesStd = (j: Int) => if (j >= 0 && j < numFeatures) featuresStd(j) else 0.0

I think either is ok, but it's better to keep consistent with other algorithms. We can change here to use function or change the third argument of L2Regularization to Option[Array[Double]]. I'm prefer the former way, what's your opinion? Thanks.

Sure. Will change it to function

yanboliang · 2017-08-17T09:59:31Z

mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/HingeAggregatorSuite.scala

+    }
+  }
+
+  test("check sizes binomial") {


Remove binomial?

yanboliang · 2017-08-17T10:00:20Z

mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/HingeAggregatorSuite.scala

+  }
+
+
+  test("check correctness binomial") {


yanboliang · 2017-08-17T10:02:08Z

mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/HingeAggregatorSuite.scala

+  }
+
+  test("check with zero standard deviation") {
+    val instancesConstantFeature = Array(


What about move the dataset to the beginning of this class?

yanboliang · 2017-08-17T10:06:01Z

@hhbyyh I think it's ready to move WIP in the PR title. We can move forward after addressing the comments. Thanks.

SparkQA · 2017-08-20T05:45:07Z

Test build #80884 has finished for PR 18315 at commit 3d7bfce.

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

sethah · 2017-08-21T16:11:40Z

mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/HingeAggregatorSuite.scala

+    assert(gradient ~== agg.gradient relTol 0.01)
+  }
+
+  test("check with zero standard deviation") {


As we found out in #18896, this test is not thorough enough. We should check all elements of the gradient for correctness.

yanboliang

Looks good overall, only some minor comments. @hhbyyh Could you address them and @sethah 's comment, then we can move forward. Thanks.

yanboliang · 2017-08-23T10:45:47Z

mllib/src/main/scala/org/apache/spark/ml/optim/aggregator/HingeAggregator.scala

+
+/**
+ * HingeAggregator computes the gradient and loss for loss function ("hinge" or
+ * "squared_hinge", as used in binary classification for instances in sparse or dense


Yeah, I agree you for separate SquaredHingeAggregator. Then we should remove squared_hinge from here?
BTW, you missed right parenthesis here.

yanboliang · 2017-08-23T10:51:02Z

mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/HingeAggregatorSuite.scala

+
+  test("aggregator add method input size") {
+    val coefArray = Array(1.0, 2.0)
+    val interceptArray = Array(2.0)


This is different from LogisticRegression which supports multi-classification, so interceptArray should be renamed to intercept?

Changing it to intercept will cause conflict with intercept[IllegalArgumentException] in the same test. Do you mind if we keep using interceptArray ?

yanboliang · 2017-08-23T10:51:25Z

mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/HingeAggregatorSuite.scala

+
+  test("negative weight") {
+    val coefArray = Array(1.0, 2.0)
+    val interceptArray = Array(2.0)


hhbyyh · 2017-08-23T20:15:06Z

Thanks for the comment @sethah and @yanboliang . Updated according to the comments.

SparkQA · 2017-08-23T21:17:06Z

Test build #81048 has finished for PR 18315 at commit 4173084.

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

yanboliang

LGTM

yanboliang · 2017-08-25T02:20:50Z

mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/HingeAggregatorSuite.scala

+
+  test("aggregator add method input size") {
+    val coefArray = Array(1.0, 2.0)
+    val interceptArray = Array(2.0)


yanboliang · 2017-08-25T02:22:14Z

Merged into master. Thanks for all.

add hinge agg

071ce88

YY-OnCall added 2 commits July 26, 2017 13:58

resolve conflict

87c7279

add unit test

c8228fb

MLnick reviewed Jul 27, 2017

View reviewed changes

add 1 un

94e0250

yanboliang reviewed Aug 17, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into svcAggregator

bc33cf9

hhbyyh changed the title ~~[SPARK-21108] [ML] [WIP] convert LinearSVC to aggregator framework~~ [SPARK-21108] [ML] convert LinearSVC to aggregator framework Aug 20, 2017

unit test update

3d7bfce

sethah reviewed Aug 21, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into svcAggregator

b19ca41

yanboliang reviewed Aug 23, 2017

View reviewed changes

YY-OnCall added 2 commits August 23, 2017 12:11

Merge remote-tracking branch 'upstream/master' into svcAggregator

30e8646

update std case

4173084

yanboliang approved these changes Aug 25, 2017

View reviewed changes

asfgit closed this in f3676d6 Aug 25, 2017

		@@ -173,7 +174,7 @@ class LinearSVCSuite extends SparkFunSuite with MLlibTestSparkContext with Defau
		test("sparse coefficients in SVCAggregator") {

[SPARK-21108] [ML] convert LinearSVC to aggregator framework #18315

[SPARK-21108] [ML] convert LinearSVC to aggregator framework #18315

Conversation

hhbyyh commented Jun 15, 2017

What changes were proposed in this pull request?

How was this patch tested?

hhbyyh commented Jun 15, 2017

SparkQA commented Jun 15, 2017

sethah commented Jun 16, 2017

sethah commented Jul 26, 2017

SparkQA commented Jul 27, 2017

Choose a reason for hiding this comment

MLnick Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

MLnick commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 28, 2017

hhbyyh commented Jul 29, 2017

SparkQA commented Aug 15, 2017

yanboliang commented Aug 16, 2017

yanboliang commented Aug 16, 2017

SparkQA commented Aug 16, 2017

yanboliang commented Aug 16, 2017

SparkQA commented Aug 16, 2017

yanboliang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanboliang commented Aug 17, 2017 • edited Loading

SparkQA commented Aug 20, 2017

Choose a reason for hiding this comment

yanboliang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhbyyh commented Aug 23, 2017 • edited Loading

SparkQA commented Aug 23, 2017

yanboliang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanboliang commented Aug 25, 2017

MLnick Jul 27, 2017 •

edited

Loading

yanboliang commented Aug 17, 2017 •

edited

Loading

hhbyyh commented Aug 23, 2017 •

edited

Loading