You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by sethah <gi...@git.apache.org> on 2017/06/14 18:54:00 UTC
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
GitHub user sethah opened a pull request:
https://github.com/apache/spark/pull/18305
[SPARK-20988][ML] Logistic regression uses aggregator hierarchy
## What changes were proposed in this pull request?
This change pulls the `LogisticAggregator` class out of LogisticRegression.scala and makes it extend `DifferentiableLossAggregator`. It also changes logistic regression to use the generic `RDDLossFunction` instead of having its own.
Other minor changes:
* L2Regularization accepts `Option[Int => Double]` for features standard deviation
* L2Regularization uses `Vector` type instead of Array
* Some tests added to LeastSquaresAggregator
## How was this patch tested?
Unit test suites are added.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sethah/spark SPARK-20988
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/18305.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #18305
----
commit 64c1d8ba26a4cf852efb09485526364323b26eb3
Author: sethah <sh...@cloudera.com>
Date: 2017-06-05T20:40:41Z
tests pass
commit a5b18c22a68418c29a13aab7a1fd00eec5176658
Author: sethah <sh...@cloudera.com>
Date: 2017-06-14T05:57:19Z
passing tests, added tests to leastsquares agg
commit 6edd12893a255c1f44416a16e42c6ef79edc8f36
Author: sethah <sh...@cloudera.com>
Date: 2017-06-14T18:26:49Z
style checker
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r128042544
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/loss/DifferentiableRegularization.scala ---
@@ -32,40 +34,45 @@ private[ml] trait DifferentiableRegularization[T] extends DiffFunction[T] {
}
/**
- * A Breeze diff function for computing the L2 regularized loss and gradient of an array of
+ * A Breeze diff function for computing the L2 regularized loss and gradient of a vector of
* coefficients.
*
* @param regParam The magnitude of the regularization.
* @param shouldApply A function (Int => Boolean) indicating whether a given index should have
* regularization applied to it.
- * @param featuresStd Option indicating whether the regularization should be scaled by the standard
- * deviation of the features.
+ * @param applyFeaturesStd Option for a function which maps coefficient index (column major) to the
+ * feature standard deviation. If `None`, no standardization is applied.
*/
private[ml] class L2Regularization(
- val regParam: Double,
+ override val regParam: Double,
shouldApply: Int => Boolean,
- featuresStd: Option[Array[Double]]) extends DifferentiableRegularization[Array[Double]] {
+ applyFeaturesStd: Option[Int => Double]) extends DifferentiableRegularization[Vector] {
- override def calculate(coefficients: Array[Double]): (Double, Array[Double]) = {
- var sum = 0.0
- val gradient = new Array[Double](coefficients.length)
- coefficients.indices.filter(shouldApply).foreach { j =>
- val coef = coefficients(j)
- featuresStd match {
- case Some(stds) =>
- val std = stds(j)
- if (std != 0.0) {
- val temp = coef / (std * std)
- sum += coef * temp
- gradient(j) = regParam * temp
- } else {
- 0.0
+ override def calculate(coefficients: Vector): (Double, Vector) = {
+ coefficients match {
+ case dv: DenseVector =>
+ var sum = 0.0
+ val gradient = new Array[Double](dv.size)
+ dv.values.indices.filter(shouldApply).foreach { j =>
+ val coef = coefficients(j)
+ applyFeaturesStd match {
+ case Some(getStd) =>
+ val std = getStd(j)
+ if (std != 0.0) {
+ val temp = coef / (std * std)
+ sum += coef * temp
+ gradient(j) = regParam * temp
+ } else {
+ 0.0
+ }
+ case None =>
+ sum += coef * coef
+ gradient(j) = coef * regParam
--- End diff --
Sorry, I don't see the point in this change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r126357847
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregatorSuite.scala ---
@@ -0,0 +1,254 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.ml.optim.aggregator
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.ml.feature.Instance
+import org.apache.spark.ml.linalg.{BLAS, Matrices, Vector, Vectors}
+import org.apache.spark.ml.util.TestingUtils._
+import org.apache.spark.mllib.util.MLlibTestSparkContext
+
+class LogisticAggregatorSuite extends SparkFunSuite with MLlibTestSparkContext {
+
+ import DifferentiableLossAggregatorSuite.getClassificationSummarizers
+
+ @transient var instances: Array[Instance] = _
+ @transient var instancesConstantFeature: Array[Instance] = _
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ instances = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.5, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(4.0, 0.5))
+ )
+ instancesConstantFeature = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.0, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(1.0, 0.5))
+ )
+ }
+
+
+ /** Get summary statistics for some data and create a new LogisticAggregator. */
+ private def getNewAggregator(
+ instances: Array[Instance],
+ coefficients: Vector,
+ fitIntercept: Boolean,
+ isMultinomial: Boolean): LogisticAggregator = {
+ val (featuresSummarizer, ySummarizer) =
+ DifferentiableLossAggregatorSuite.getClassificationSummarizers(instances)
+ val numClasses = ySummarizer.histogram.length
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val bcFeaturesStd = spark.sparkContext.broadcast(featuresStd)
+ val bcCoefficients = spark.sparkContext.broadcast(coefficients)
--- End diff --
I guess it's done now - but is it really necessary? Do we do that in other tests explicitly? Since typically the spark context / session is torn down after each test suite I don't think it's a real issue.
It adds a whole bunch of code that just serves to obfuscate the actual test cases.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on the issue:
https://github.com/apache/spark/pull/18305
Did we reach a consensus on the broadcast variables? My opinion is that it's probably better in this case not to worry about it, and we can back out the change that destroys them in the test suites.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/18305
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on the issue:
https://github.com/apache/spark/pull/18305
cc @VinceShieh @MLnick @srowen
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18305
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18305
**[Test build #78820 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78820/testReport)** for PR 18305 at commit [`a19b385`](https://github.com/apache/spark/commit/a19b385913f450254b6849098f68cab4c2dac618).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18305
**[Test build #79962 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79962/testReport)** for PR 18305 at commit [`cb40b31`](https://github.com/apache/spark/commit/cb40b3189b546283635683d7711e0e2e28366034).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r125026593
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/loss/RDDLossFunction.scala ---
@@ -62,8 +62,8 @@ private[ml] class RDDLossFunction[
val newAgg = instances.treeAggregate(thisAgg)(seqOp, combOp, aggregationDepth)
val gradient = newAgg.gradient
val regLoss = regularization.map { regFun =>
- val (regLoss, regGradient) = regFun.calculate(coefficients.data)
- BLAS.axpy(1.0, Vectors.dense(regGradient), gradient)
+ val (regLoss, regGradient) = regFun.calculate(Vectors.fromBreeze(coefficients))
--- End diff --
Yeah, more or less - see how Breeze does L2 reg in `DiffFunction.withL2Regularization`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r126446952
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregatorSuite.scala ---
@@ -0,0 +1,254 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.ml.optim.aggregator
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.ml.feature.Instance
+import org.apache.spark.ml.linalg.{BLAS, Matrices, Vector, Vectors}
+import org.apache.spark.ml.util.TestingUtils._
+import org.apache.spark.mllib.util.MLlibTestSparkContext
+
+class LogisticAggregatorSuite extends SparkFunSuite with MLlibTestSparkContext {
+
+ import DifferentiableLossAggregatorSuite.getClassificationSummarizers
+
+ @transient var instances: Array[Instance] = _
+ @transient var instancesConstantFeature: Array[Instance] = _
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ instances = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.5, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(4.0, 0.5))
+ )
+ instancesConstantFeature = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.0, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(1.0, 0.5))
+ )
+ }
+
+
+ /** Get summary statistics for some data and create a new LogisticAggregator. */
+ private def getNewAggregator(
+ instances: Array[Instance],
+ coefficients: Vector,
+ fitIntercept: Boolean,
+ isMultinomial: Boolean): LogisticAggregator = {
+ val (featuresSummarizer, ySummarizer) =
+ DifferentiableLossAggregatorSuite.getClassificationSummarizers(instances)
+ val numClasses = ySummarizer.histogram.length
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val bcFeaturesStd = spark.sparkContext.broadcast(featuresStd)
+ val bcCoefficients = spark.sparkContext.broadcast(coefficients)
--- End diff --
Thoughts @yanboliang? Nick may have a valid point here - the size of these broadcast variables should be very small, and they'll get destroyed along with the SparkContext after every test suite. Is there something else we haven't thought of?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18305
**[Test build #79715 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79715/testReport)** for PR 18305 at commit [`cb40b31`](https://github.com/apache/spark/commit/cb40b3189b546283635683d7711e0e2e28366034).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on the issue:
https://github.com/apache/spark/pull/18305
Jenkins retest this please
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18305
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/78063/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on the issue:
https://github.com/apache/spark/pull/18305
Overall looks good, left a few comments about the reg function change and tests for logistic agg.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r124240666
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregatorSuite.scala ---
@@ -0,0 +1,254 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.ml.optim.aggregator
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.ml.feature.Instance
+import org.apache.spark.ml.linalg.{BLAS, Matrices, Vector, Vectors}
+import org.apache.spark.ml.util.TestingUtils._
+import org.apache.spark.mllib.util.MLlibTestSparkContext
+
+class LogisticAggregatorSuite extends SparkFunSuite with MLlibTestSparkContext {
+
+ import DifferentiableLossAggregatorSuite.getClassificationSummarizers
+
+ @transient var instances: Array[Instance] = _
+ @transient var instancesConstantFeature: Array[Instance] = _
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ instances = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.5, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(4.0, 0.5))
+ )
+ instancesConstantFeature = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.0, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(1.0, 0.5))
+ )
+ }
+
+
+ /** Get summary statistics for some data and create a new LogisticAggregator. */
+ private def getNewAggregator(
+ instances: Array[Instance],
+ coefficients: Vector,
+ fitIntercept: Boolean,
+ isMultinomial: Boolean): LogisticAggregator = {
+ val (featuresSummarizer, ySummarizer) =
+ DifferentiableLossAggregatorSuite.getClassificationSummarizers(instances)
+ val numClasses = ySummarizer.histogram.length
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val bcFeaturesStd = spark.sparkContext.broadcast(featuresStd)
+ val bcCoefficients = spark.sparkContext.broadcast(coefficients)
+ new LogisticAggregator(bcFeaturesStd, numClasses, fitIntercept, isMultinomial)(bcCoefficients)
+ }
+
+ test("aggregator add method input size") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator features dimension must match coefficients dimension") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, 1.0, Vectors.dense(2.0)))
+ }
+ }
+ }
+
+ test("negative weight") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator does not support negative instance weights") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, -1.0, Vectors.dense(2.0, 1.0)))
+ }
+ }
+ }
+
+ test("check sizes multinomial") {
+ val rng = new scala.util.Random
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val coefWithIntercept = Vectors.dense(
+ Array.fill(numClasses * (numFeatures + 1))(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(
+ Array.fill(numClasses * numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(instances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = true)
+ val aggNoIntercept = getNewAggregator(instances, coefWithoutIntercept, fitIntercept = false,
+ isMultinomial = true)
+ instances.foreach(aggIntercept.add)
+ instances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === (numFeatures + 1) * numClasses)
+ assert(aggNoIntercept.gradient.size === numFeatures * numClasses)
+ }
+
+ test("check sizes binomial") {
+ val rng = new scala.util.Random
+ val binaryInstances = instances.filter(_.label < 2.0)
+ val numFeatures = binaryInstances.head.features.size
+ val coefWithIntercept = Vectors.dense(Array.fill(numFeatures + 1)(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(Array.fill(numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(binaryInstances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = false)
+ val aggNoIntercept = getNewAggregator(binaryInstances, coefWithoutIntercept,
+ fitIntercept = false, isMultinomial = false)
+ binaryInstances.foreach(aggIntercept.add)
+ binaryInstances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === numFeatures + 1)
+ assert(aggNoIntercept.gradient.size === numFeatures)
+ }
+
+ test("check correctness multinomial") {
+ /*
+ Check that the aggregator computes loss/gradient for:
+ -sum_i w_i * (beta_y dot x_i - log(sum_k e^(beta_k dot x_i)))
+ */
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val intercepts = Vectors.dense(interceptArray)
+ val (featuresSummarizer, ySummarizer) = getClassificationSummarizers(instances)
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val weightSum = instances.map(_.weight).sum
+
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ instances.foreach(agg.add)
+
+ // compute the loss
+ val stdCoef = coefArray.indices.map(i => coefArray(i) / featuresStd(i / numClasses)).toArray
--- End diff --
Should we not also test the non-standardized path?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r124241031
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/loss/RDDLossFunctionSuite.scala ---
@@ -46,11 +46,11 @@ class RDDLossFunctionSuite extends SparkFunSuite with MLlibTestSparkContext {
val lossWithReg = new RDDLossFunction(instances, getAgg, Some(regLossFun))
val (loss1, grad1) = lossNoReg.calculate(coefficients.asBreeze.toDenseVector)
- val (regLoss, regGrad) = regLossFun.calculate(coefficients.toArray)
+ val (regLoss, regGrad) = regLossFun.calculate(coefficients)
--- End diff --
Again this serves to highlight the inconsistency - the loss fn and reg loss fn should be composable IMO.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r127874833
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/loss/DifferentiableRegularization.scala ---
@@ -32,40 +34,45 @@ private[ml] trait DifferentiableRegularization[T] extends DiffFunction[T] {
}
/**
- * A Breeze diff function for computing the L2 regularized loss and gradient of an array of
+ * A Breeze diff function for computing the L2 regularized loss and gradient of a vector of
* coefficients.
*
* @param regParam The magnitude of the regularization.
* @param shouldApply A function (Int => Boolean) indicating whether a given index should have
* regularization applied to it.
- * @param featuresStd Option indicating whether the regularization should be scaled by the standard
- * deviation of the features.
+ * @param applyFeaturesStd Option for a function which maps coefficient index (column major) to the
+ * feature standard deviation. If `None`, no standardization is applied.
*/
private[ml] class L2Regularization(
- val regParam: Double,
+ override val regParam: Double,
shouldApply: Int => Boolean,
- featuresStd: Option[Array[Double]]) extends DifferentiableRegularization[Array[Double]] {
+ applyFeaturesStd: Option[Int => Double]) extends DifferentiableRegularization[Vector] {
- override def calculate(coefficients: Array[Double]): (Double, Array[Double]) = {
- var sum = 0.0
- val gradient = new Array[Double](coefficients.length)
- coefficients.indices.filter(shouldApply).foreach { j =>
- val coef = coefficients(j)
- featuresStd match {
- case Some(stds) =>
- val std = stds(j)
- if (std != 0.0) {
- val temp = coef / (std * std)
- sum += coef * temp
- gradient(j) = regParam * temp
- } else {
- 0.0
+ override def calculate(coefficients: Vector): (Double, Vector) = {
+ coefficients match {
+ case dv: DenseVector =>
+ var sum = 0.0
+ val gradient = new Array[Double](dv.size)
+ dv.values.indices.filter(shouldApply).foreach { j =>
+ val coef = coefficients(j)
+ applyFeaturesStd match {
+ case Some(getStd) =>
+ val std = getStd(j)
+ if (std != 0.0) {
+ val temp = coef / (std * std)
+ sum += coef * temp
+ gradient(j) = regParam * temp
+ } else {
+ 0.0
+ }
+ case None =>
+ sum += coef * coef
+ gradient(j) = coef * regParam
--- End diff --
Trivial, to match `regParam * temp` above, how about using `regParam * coef`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18305
**[Test build #79962 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79962/testReport)** for PR 18305 at commit [`cb40b31`](https://github.com/apache/spark/commit/cb40b3189b546283635683d7711e0e2e28366034).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18305
**[Test build #78065 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78065/testReport)** for PR 18305 at commit [`fcf5372`](https://github.com/apache/spark/commit/fcf537208f25dac0b4fffa35f875944d82fdb180).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18305
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/78820/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18305
**[Test build #79244 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79244/testReport)** for PR 18305 at commit [`a51e565`](https://github.com/apache/spark/commit/a51e56512b608ed17b356b4672ef11113a031126).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18305
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/79962/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r124016584
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/loss/RDDLossFunction.scala ---
@@ -50,7 +50,7 @@ private[ml] class RDDLossFunction[
Agg <: DifferentiableLossAggregator[T, Agg]: ClassTag](
instances: RDD[T],
getAggregator: (Broadcast[Vector] => Agg),
- regularization: Option[DifferentiableRegularization[Array[Double]]],
+ regularization: Option[DifferentiableRegularization[Vector]],
--- End diff --
This change is just to make things more generic, yes?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by hhbyyh <gi...@git.apache.org>.
Github user hhbyyh commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r124135629
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/loss/DifferentiableRegularization.scala ---
@@ -38,34 +40,39 @@ private[ml] trait DifferentiableRegularization[T] extends DiffFunction[T] {
* @param regParam The magnitude of the regularization.
--- End diff --
The comments for the function need to be updated. (It still says array of coefficients)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by hhbyyh <gi...@git.apache.org>.
Github user hhbyyh commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r124133920
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregator.scala ---
@@ -0,0 +1,364 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.ml.optim.aggregator
+
+import org.apache.spark.broadcast.Broadcast
+import org.apache.spark.internal.Logging
+import org.apache.spark.ml.feature.Instance
+import org.apache.spark.ml.linalg.{DenseVector, Vector}
+import org.apache.spark.mllib.util.MLUtils
+
+/**
+ * LogisticAggregator computes the gradient and loss for binary or multinomial logistic (softmax)
+ * loss function, as used in classification for instances in sparse or dense vector in an online
+ * fashion.
+ *
+ * Two LogisticAggregators can be merged together to have a summary of loss and gradient of
+ * the corresponding joint dataset.
+ *
+ * For improving the convergence rate during the optimization process and also to prevent against
+ * features with very large variances exerting an overly large influence during model training,
+ * packages like R's GLMNET perform the scaling to unit variance and remove the mean in order to
+ * reduce the condition number. The model is then trained in this scaled space, but returns the
+ * coefficients in the original scale. See page 9 in
+ * http://cran.r-project.org/web/packages/glmnet/glmnet.pdf
+ *
+ * However, we don't want to apply the [[org.apache.spark.ml.feature.StandardScaler]] on the
+ * training dataset, and then cache the standardized dataset since it will create a lot of overhead.
+ * As a result, we perform the scaling implicitly when we compute the objective function (though
+ * we do not subtract the mean).
+ *
+ * Note that there is a difference between multinomial (softmax) and binary loss. The binary case
+ * uses one outcome class as a "pivot" and regresses the other class against the pivot. In the
+ * multinomial case, the softmax loss function is used to model each class probability
+ * independently. Using softmax loss produces `K` sets of coefficients, while using a pivot class
+ * produces `K - 1` sets of coefficients (a single coefficient vector in the binary case). In the
+ * binary case, we can say that the coefficients are shared between the positive and negative
+ * classes. When regularization is applied, multinomial (softmax) loss will produce a result
+ * different from binary loss since the positive and negative don't share the coefficients while the
+ * binary regression shares the coefficients between positive and negative.
+ *
+ * The following is a mathematical derivation for the multinomial (softmax) loss.
+ *
+ * The probability of the multinomial outcome $y$ taking on any of the K possible outcomes is:
+ *
+ * <blockquote>
+ * $$
+ * P(y_i=0|\vec{x}_i, \beta) = \frac{e^{\vec{x}_i^T \vec{\beta}_0}}{\sum_{k=0}^{K-1}
+ * e^{\vec{x}_i^T \vec{\beta}_k}} \\
+ * P(y_i=1|\vec{x}_i, \beta) = \frac{e^{\vec{x}_i^T \vec{\beta}_1}}{\sum_{k=0}^{K-1}
+ * e^{\vec{x}_i^T \vec{\beta}_k}}\\
+ * P(y_i=K-1|\vec{x}_i, \beta) = \frac{e^{\vec{x}_i^T \vec{\beta}_{K-1}}\,}{\sum_{k=0}^{K-1}
+ * e^{\vec{x}_i^T \vec{\beta}_k}}
+ * $$
+ * </blockquote>
+ *
+ * The model coefficients $\beta = (\beta_0, \beta_1, \beta_2, ..., \beta_{K-1})$ become a matrix
+ * which has dimension of $K \times (N+1)$ if the intercepts are added. If the intercepts are not
+ * added, the dimension will be $K \times N$.
+ *
+ * Note that the coefficients in the model above lack identifiability. That is, any constant scalar
+ * can be added to all of the coefficients and the probabilities remain the same.
+ *
+ * <blockquote>
+ * $$
+ * \begin{align}
+ * \frac{e^{\vec{x}_i^T \left(\vec{\beta}_0 + \vec{c}\right)}}{\sum_{k=0}^{K-1}
+ * e^{\vec{x}_i^T \left(\vec{\beta}_k + \vec{c}\right)}}
+ * = \frac{e^{\vec{x}_i^T \vec{\beta}_0}e^{\vec{x}_i^T \vec{c}}\,}{e^{\vec{x}_i^T \vec{c}}
+ * \sum_{k=0}^{K-1} e^{\vec{x}_i^T \vec{\beta}_k}}
+ * = \frac{e^{\vec{x}_i^T \vec{\beta}_0}}{\sum_{k=0}^{K-1} e^{\vec{x}_i^T \vec{\beta}_k}}
+ * \end{align}
+ * $$
+ * </blockquote>
+ *
+ * However, when regularization is added to the loss function, the coefficients are indeed
+ * identifiable because there is only one set of coefficients which minimizes the regularization
+ * term. When no regularization is applied, we choose the coefficients with the minimum L2
+ * penalty for consistency and reproducibility. For further discussion see:
+ *
+ * Friedman, et al. "Regularization Paths for Generalized Linear Models via Coordinate Descent"
+ *
+ * The loss of objective function for a single instance of data (we do not include the
+ * regularization term here for simplicity) can be written as
+ *
+ * <blockquote>
+ * $$
+ * \begin{align}
+ * \ell\left(\beta, x_i\right) &= -log{P\left(y_i \middle| \vec{x}_i, \beta\right)} \\
+ * &= log\left(\sum_{k=0}^{K-1}e^{\vec{x}_i^T \vec{\beta}_k}\right) - \vec{x}_i^T \vec{\beta}_y\\
+ * &= log\left(\sum_{k=0}^{K-1} e^{margins_k}\right) - margins_y
+ * \end{align}
+ * $$
+ * </blockquote>
+ *
+ * where ${margins}_k = \vec{x}_i^T \vec{\beta}_k$.
+ *
+ * For optimization, we have to calculate the first derivative of the loss function, and a simple
+ * calculation shows that
+ *
+ * <blockquote>
+ * $$
+ * \begin{align}
+ * \frac{\partial \ell(\beta, \vec{x}_i, w_i)}{\partial \beta_{j, k}}
+ * &= x_{i,j} \cdot w_i \cdot \left(\frac{e^{\vec{x}_i \cdot \vec{\beta}_k}}{\sum_{k'=0}^{K-1}
+ * e^{\vec{x}_i \cdot \vec{\beta}_{k'}}\,} - I_{y=k}\right) \\
+ * &= x_{i, j} \cdot w_i \cdot multiplier_k
+ * \end{align}
+ * $$
+ * </blockquote>
+ *
+ * where $w_i$ is the sample weight, $I_{y=k}$ is an indicator function
+ *
+ * <blockquote>
+ * $$
+ * I_{y=k} = \begin{cases}
+ * 1 & y = k \\
+ * 0 & else
+ * \end{cases}
+ * $$
+ * </blockquote>
+ *
+ * and
+ *
+ * <blockquote>
+ * $$
+ * multiplier_k = \left(\frac{e^{\vec{x}_i \cdot \vec{\beta}_k}}{\sum_{k=0}^{K-1}
+ * e^{\vec{x}_i \cdot \vec{\beta}_k}} - I_{y=k}\right)
+ * $$
+ * </blockquote>
+ *
+ * If any of margins is larger than 709.78, the numerical computation of multiplier and loss
+ * function will suffer from arithmetic overflow. This issue occurs when there are outliers in
+ * data which are far away from the hyperplane, and this will cause the failing of training once
+ * infinity is introduced. Note that this is only a concern when max(margins) > 0.
+ *
+ * Fortunately, when max(margins) = maxMargin > 0, the loss function and the multiplier can
+ * easily be rewritten into the following equivalent numerically stable formula.
+ *
+ * <blockquote>
+ * $$
+ * \ell\left(\beta, x\right) = log\left(\sum_{k=0}^{K-1} e^{margins_k - maxMargin}\right) -
+ * margins_{y} + maxMargin
+ * $$
+ * </blockquote>
+ *
+ * Note that each term, $(margins_k - maxMargin)$ in the exponential is no greater than zero; as a
+ * result, overflow will not happen with this formula.
+ *
+ * For $multiplier$, a similar trick can be applied as the following,
+ *
+ * <blockquote>
+ * $$
+ * multiplier_k = \left(\frac{e^{\vec{x}_i \cdot \vec{\beta}_k - maxMargin}}{\sum_{k'=0}^{K-1}
+ * e^{\vec{x}_i \cdot \vec{\beta}_{k'} - maxMargin}} - I_{y=k}\right)
+ * $$
+ * </blockquote>
+ *
+ *
+ * @param bcCoefficients The broadcast coefficients corresponding to the features.
+ * @param bcFeaturesStd The broadcast standard deviation values of the features.
+ * @param numClasses the number of possible outcomes for k classes classification problem in
+ * Multinomial Logistic Regression.
+ * @param fitIntercept Whether to fit an intercept term.
+ * @param multinomial Whether to use multinomial (softmax) or binary loss
+ * @note In order to avoid unnecessary computation during calculation of the gradient updates
+ * we lay out the coefficients in column major order during training. This allows us to
+ * perform feature standardization once, while still retaining sequential memory access
+ * for speed. We convert back to row major order when we create the model,
+ * since this form is optimal for the matrix operations used for prediction.
+ */
+private[ml] class LogisticAggregator(
+ bcFeaturesStd: Broadcast[Array[Double]],
+ numClasses: Int,
+ fitIntercept: Boolean,
+ multinomial: Boolean)(bcCoefficients: Broadcast[Vector])
+ extends DifferentiableLossAggregator[Instance, LogisticAggregator] with Logging {
+
+ private val numFeatures = bcFeaturesStd.value.length
+ private val numFeaturesPlusIntercept = if (fitIntercept) numFeatures + 1 else numFeatures
+ private val coefficientSize = bcCoefficients.value.size
+ protected override val dim: Int = coefficientSize
+ if (multinomial) {
+ require(numClasses == coefficientSize / numFeaturesPlusIntercept, s"The number of " +
+ s"coefficients should be ${numClasses * numFeaturesPlusIntercept} but was $coefficientSize")
+ } else {
+ require(coefficientSize == numFeaturesPlusIntercept, s"Expected $numFeaturesPlusIntercept " +
+ s"coefficients but got $coefficientSize")
+ require(numClasses == 1 || numClasses == 2, s"Binary logistic aggregator requires numClasses " +
+ s"in {1, 2} but found $numClasses.")
+ }
+
+ @transient private lazy val coefficientsArray: Array[Double] = bcCoefficients.value match {
+ case DenseVector(values) => values
+ case _ => throw new IllegalArgumentException(s"coefficients only supports dense vector but " +
+ s"got type ${bcCoefficients.value.getClass}.)")
+ }
+
+ if (multinomial && numClasses <= 2) {
+ logInfo(s"Multinomial logistic regression for binary classification yields separate " +
+ s"coefficients for positive and negative classes. When no regularization is applied, the" +
+ s"result will be effectively the same as binary logistic regression. When regularization" +
+ s"is applied, multinomial loss will produce a result different from binary loss.")
+ }
+
+ /** Update gradient and loss using binary loss function. */
+ private def binaryUpdateInPlace(features: Vector, weight: Double, label: Double): Unit = {
+
+ val localFeaturesStd = bcFeaturesStd.value
+ val localCoefficients = coefficientsArray
+ val localGradientArray = gradientSumArray
+ val margin = - {
+ var sum = 0.0
+ features.foreachActive { (index, value) =>
+ if (localFeaturesStd(index) != 0.0 && value != 0.0) {
+ sum += localCoefficients(index) * value / localFeaturesStd(index)
+ }
+ }
+ if (fitIntercept) sum += localCoefficients(numFeaturesPlusIntercept - 1)
+ sum
+ }
+
+ val multiplier = weight * (1.0 / (1.0 + math.exp(margin)) - label)
+
+ features.foreachActive { (index, value) =>
+ if (localFeaturesStd(index) != 0.0 && value != 0.0) {
+ localGradientArray(index) += multiplier * value / localFeaturesStd(index)
+ }
+ }
+
+ if (fitIntercept) {
+ localGradientArray(numFeaturesPlusIntercept - 1) += multiplier
+ }
+
+ if (label > 0) {
+ // The following is equivalent to log(1 + exp(margin)) but more numerically stable.
+ lossSum += weight * MLUtils.log1pExp(margin)
+ } else {
+ lossSum += weight * (MLUtils.log1pExp(margin) - margin)
+ }
+ }
+
+ /** Update gradient and loss using multinomial (softmax) loss function. */
+ private def multinomialUpdateInPlace(features: Vector, weight: Double, label: Double): Unit = {
+ // TODO: use level 2 BLAS operations
+ /*
+ Note: this can still be used when numClasses = 2 for binary
+ logistic regression without pivoting.
+ */
+ val localFeaturesStd = bcFeaturesStd.value
+ val localCoefficients = coefficientsArray
+ val localGradientArray = gradientSumArray
+
+ // marginOfLabel is margins(label) in the formula
+ var marginOfLabel = 0.0
+ var maxMargin = Double.NegativeInfinity
+
+ val margins = new Array[Double](numClasses)
+ features.foreachActive { (index, value) =>
+ val stdValue = value / localFeaturesStd(index)
+ var j = 0
+ while (j < numClasses) {
+ margins(j) += localCoefficients(index * numClasses + j) * stdValue
+ j += 1
+ }
+ }
+ var i = 0
+ while (i < numClasses) {
+ if (fitIntercept) {
+ margins(i) += localCoefficients(numClasses * numFeatures + i)
+ }
+ if (i == label.toInt) marginOfLabel = margins(i)
+ if (margins(i) > maxMargin) {
+ maxMargin = margins(i)
+ }
+ i += 1
+ }
+
+ /**
+ * When maxMargin is greater than 0, the original formula could cause overflow.
+ * We address this by subtracting maxMargin from all the margins, so it's guaranteed
+ * that all of the new margins will be smaller than zero to prevent arithmetic overflow.
+ */
+ val multipliers = new Array[Double](numClasses)
+ val sum = {
+ var temp = 0.0
+ var i = 0
+ while (i < numClasses) {
+ if (maxMargin > 0) margins(i) -= maxMargin
+ val exp = math.exp(margins(i))
+ temp += exp
+ multipliers(i) = exp
+ i += 1
+ }
+ temp
+ }
+
+ margins.indices.foreach { i =>
+ multipliers(i) = multipliers(i) / sum - (if (label == i) 1.0 else 0.0)
+ }
+ features.foreachActive { (index, value) =>
+ if (localFeaturesStd(index) != 0.0 && value != 0.0) {
+ val stdValue = value / localFeaturesStd(index)
+ var j = 0
+ while (j < numClasses) {
+ localGradientArray(index * numClasses + j) += weight * multipliers(j) * stdValue
+ j += 1
+ }
+ }
+ }
+ if (fitIntercept) {
+ var i = 0
+ while (i < numClasses) {
+ localGradientArray(numFeatures * numClasses + i) += weight * multipliers(i)
+ i += 1
+ }
+ }
+
+ val loss = if (maxMargin > 0) {
+ math.log(sum) - marginOfLabel + maxMargin
+ } else {
+ math.log(sum) - marginOfLabel
+ }
+ lossSum += weight * loss
+ }
+
+ /**
+ * Add a new training instance to this LogisticAggregator, and update the loss and gradient
+ * of the objective function.
+ *
+ * @param instance The instance of data point to be added.
+ * @return This LogisticAggregator object.
+ */
+ def add(instance: Instance): this.type = {
+ instance match { case Instance(label, weight, features) =>
+ require(numFeatures == features.size, s"Dimensions mismatch when adding new instance." +
+ s" Expecting $numFeatures but got ${features.size}.")
+ require(weight >= 0.0, s"instance weight, $weight has to be >= 0.0")
+
+ if (weight == 0.0) return this
+
+ if (multinomial) {
+ multinomialUpdateInPlace(features, weight, label)
+ } else {
+ binaryUpdateInPlace(features, weight, label)
+ }
+ weightSum += weight
+ this
+ }
+ }
--- End diff --
For the convenience of other reviewers, I checked this part and the new implementation delegates the merge, gradient, weight and loss to the common implementation in DifferentiableLossAggregator, and made no further modification from the original implementation. LGTM.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on the issue:
https://github.com/apache/spark/pull/18305
What's blocking us here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on the issue:
https://github.com/apache/spark/pull/18305
@sethah I will take a look in a few days after some backlog, thanks for your patience.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r125680954
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregatorSuite.scala ---
@@ -0,0 +1,254 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.ml.optim.aggregator
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.ml.feature.Instance
+import org.apache.spark.ml.linalg.{BLAS, Matrices, Vector, Vectors}
+import org.apache.spark.ml.util.TestingUtils._
+import org.apache.spark.mllib.util.MLlibTestSparkContext
+
+class LogisticAggregatorSuite extends SparkFunSuite with MLlibTestSparkContext {
+
+ import DifferentiableLossAggregatorSuite.getClassificationSummarizers
+
+ @transient var instances: Array[Instance] = _
+ @transient var instancesConstantFeature: Array[Instance] = _
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ instances = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.5, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(4.0, 0.5))
+ )
+ instancesConstantFeature = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.0, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(1.0, 0.5))
+ )
+ }
+
+
+ /** Get summary statistics for some data and create a new LogisticAggregator. */
+ private def getNewAggregator(
+ instances: Array[Instance],
+ coefficients: Vector,
+ fitIntercept: Boolean,
+ isMultinomial: Boolean): LogisticAggregator = {
+ val (featuresSummarizer, ySummarizer) =
+ DifferentiableLossAggregatorSuite.getClassificationSummarizers(instances)
+ val numClasses = ySummarizer.histogram.length
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val bcFeaturesStd = spark.sparkContext.broadcast(featuresStd)
+ val bcCoefficients = spark.sparkContext.broadcast(coefficients)
+ new LogisticAggregator(bcFeaturesStd, numClasses, fitIntercept, isMultinomial)(bcCoefficients)
+ }
+
+ test("aggregator add method input size") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator features dimension must match coefficients dimension") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, 1.0, Vectors.dense(2.0)))
+ }
+ }
+ }
+
+ test("negative weight") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator does not support negative instance weights") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, -1.0, Vectors.dense(2.0, 1.0)))
+ }
+ }
+ }
+
+ test("check sizes multinomial") {
+ val rng = new scala.util.Random
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val coefWithIntercept = Vectors.dense(
+ Array.fill(numClasses * (numFeatures + 1))(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(
+ Array.fill(numClasses * numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(instances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = true)
+ val aggNoIntercept = getNewAggregator(instances, coefWithoutIntercept, fitIntercept = false,
+ isMultinomial = true)
+ instances.foreach(aggIntercept.add)
+ instances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === (numFeatures + 1) * numClasses)
+ assert(aggNoIntercept.gradient.size === numFeatures * numClasses)
+ }
+
+ test("check sizes binomial") {
+ val rng = new scala.util.Random
+ val binaryInstances = instances.filter(_.label < 2.0)
+ val numFeatures = binaryInstances.head.features.size
+ val coefWithIntercept = Vectors.dense(Array.fill(numFeatures + 1)(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(Array.fill(numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(binaryInstances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = false)
+ val aggNoIntercept = getNewAggregator(binaryInstances, coefWithoutIntercept,
+ fitIntercept = false, isMultinomial = false)
+ binaryInstances.foreach(aggIntercept.add)
+ binaryInstances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === numFeatures + 1)
+ assert(aggNoIntercept.gradient.size === numFeatures)
+ }
+
+ test("check correctness multinomial") {
+ /*
+ Check that the aggregator computes loss/gradient for:
+ -sum_i w_i * (beta_y dot x_i - log(sum_k e^(beta_k dot x_i)))
+ */
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val intercepts = Vectors.dense(interceptArray)
+ val (featuresSummarizer, ySummarizer) = getClassificationSummarizers(instances)
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val weightSum = instances.map(_.weight).sum
+
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ instances.foreach(agg.add)
+
+ // compute the loss
+ val stdCoef = coefArray.indices.map(i => coefArray(i) / featuresStd(i / numClasses)).toArray
--- End diff --
The `LogisticAggregator` is agnostic to standardization. There is no option to turn standardization off or on for the LogisticAggregator. In logistic regression, turning it off or on affects only the regularization - and so it is tested there.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r128458132
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregatorSuite.scala ---
@@ -0,0 +1,254 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.ml.optim.aggregator
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.ml.feature.Instance
+import org.apache.spark.ml.linalg.{BLAS, Matrices, Vector, Vectors}
+import org.apache.spark.ml.util.TestingUtils._
+import org.apache.spark.mllib.util.MLlibTestSparkContext
+
+class LogisticAggregatorSuite extends SparkFunSuite with MLlibTestSparkContext {
+
+ import DifferentiableLossAggregatorSuite.getClassificationSummarizers
+
+ @transient var instances: Array[Instance] = _
+ @transient var instancesConstantFeature: Array[Instance] = _
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ instances = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.5, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(4.0, 0.5))
+ )
+ instancesConstantFeature = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.0, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(1.0, 0.5))
+ )
+ }
+
+
+ /** Get summary statistics for some data and create a new LogisticAggregator. */
+ private def getNewAggregator(
+ instances: Array[Instance],
+ coefficients: Vector,
+ fitIntercept: Boolean,
+ isMultinomial: Boolean): LogisticAggregator = {
+ val (featuresSummarizer, ySummarizer) =
+ DifferentiableLossAggregatorSuite.getClassificationSummarizers(instances)
+ val numClasses = ySummarizer.histogram.length
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val bcFeaturesStd = spark.sparkContext.broadcast(featuresStd)
+ val bcCoefficients = spark.sparkContext.broadcast(coefficients)
+ new LogisticAggregator(bcFeaturesStd, numClasses, fitIntercept, isMultinomial)(bcCoefficients)
+ }
+
+ test("aggregator add method input size") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator features dimension must match coefficients dimension") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, 1.0, Vectors.dense(2.0)))
+ }
+ }
+ }
+
+ test("negative weight") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator does not support negative instance weights") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, -1.0, Vectors.dense(2.0, 1.0)))
+ }
+ }
+ }
+
+ test("check sizes multinomial") {
+ val rng = new scala.util.Random
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val coefWithIntercept = Vectors.dense(
+ Array.fill(numClasses * (numFeatures + 1))(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(
+ Array.fill(numClasses * numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(instances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = true)
+ val aggNoIntercept = getNewAggregator(instances, coefWithoutIntercept, fitIntercept = false,
+ isMultinomial = true)
+ instances.foreach(aggIntercept.add)
+ instances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === (numFeatures + 1) * numClasses)
+ assert(aggNoIntercept.gradient.size === numFeatures * numClasses)
+ }
+
+ test("check sizes binomial") {
+ val rng = new scala.util.Random
+ val binaryInstances = instances.filter(_.label < 2.0)
+ val numFeatures = binaryInstances.head.features.size
+ val coefWithIntercept = Vectors.dense(Array.fill(numFeatures + 1)(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(Array.fill(numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(binaryInstances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = false)
+ val aggNoIntercept = getNewAggregator(binaryInstances, coefWithoutIntercept,
+ fitIntercept = false, isMultinomial = false)
+ binaryInstances.foreach(aggIntercept.add)
+ binaryInstances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === numFeatures + 1)
+ assert(aggNoIntercept.gradient.size === numFeatures)
+ }
+
+ test("check correctness multinomial") {
+ /*
+ Check that the aggregator computes loss/gradient for:
+ -sum_i w_i * (beta_y dot x_i - log(sum_k e^(beta_k dot x_i)))
+ */
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val intercepts = Vectors.dense(interceptArray)
+ val (featuresSummarizer, ySummarizer) = getClassificationSummarizers(instances)
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val weightSum = instances.map(_.weight).sum
+
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ instances.foreach(agg.add)
+
+ // compute the loss
+ val stdCoef = coefArray.indices.map(i => coefArray(i) / featuresStd(i / numClasses)).toArray
--- End diff --
Yeah it's not necessary to discuss here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on the issue:
https://github.com/apache/spark/pull/18305
@sethah IMO we should back out the test-related bc var explicit destroy code as it complicates things. I hear that this _may_ help catch bugs... but frankly I'm not convinced.
Because the code setup & path in the source may not be quite the same as in the tests (almost never I'd say), I don't believe you will necessarily catch bugs such as the one mentioned by Yanbo.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18305
**[Test build #79244 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79244/testReport)** for PR 18305 at commit [`a51e565`](https://github.com/apache/spark/commit/a51e56512b608ed17b356b4672ef11113a031126).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18305
**[Test build #78065 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78065/testReport)** for PR 18305 at commit [`fcf5372`](https://github.com/apache/spark/commit/fcf537208f25dac0b4fffa35f875944d82fdb180).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r125675084
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregatorSuite.scala ---
@@ -0,0 +1,254 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.ml.optim.aggregator
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.ml.feature.Instance
+import org.apache.spark.ml.linalg.{BLAS, Matrices, Vector, Vectors}
+import org.apache.spark.ml.util.TestingUtils._
+import org.apache.spark.mllib.util.MLlibTestSparkContext
+
+class LogisticAggregatorSuite extends SparkFunSuite with MLlibTestSparkContext {
+
+ import DifferentiableLossAggregatorSuite.getClassificationSummarizers
+
+ @transient var instances: Array[Instance] = _
+ @transient var instancesConstantFeature: Array[Instance] = _
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ instances = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.5, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(4.0, 0.5))
+ )
+ instancesConstantFeature = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.0, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(1.0, 0.5))
+ )
+ }
+
+
+ /** Get summary statistics for some data and create a new LogisticAggregator. */
+ private def getNewAggregator(
+ instances: Array[Instance],
+ coefficients: Vector,
+ fitIntercept: Boolean,
+ isMultinomial: Boolean): LogisticAggregator = {
+ val (featuresSummarizer, ySummarizer) =
+ DifferentiableLossAggregatorSuite.getClassificationSummarizers(instances)
+ val numClasses = ySummarizer.histogram.length
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val bcFeaturesStd = spark.sparkContext.broadcast(featuresStd)
+ val bcCoefficients = spark.sparkContext.broadcast(coefficients)
--- End diff --
It's better to destroy these broadcast variable explicitly even in test suites.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r124384213
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/loss/RDDLossFunction.scala ---
@@ -62,8 +62,8 @@ private[ml] class RDDLossFunction[
val newAgg = instances.treeAggregate(thisAgg)(seqOp, combOp, aggregationDepth)
val gradient = newAgg.gradient
val regLoss = regularization.map { regFun =>
- val (regLoss, regGradient) = regFun.calculate(coefficients.data)
- BLAS.axpy(1.0, Vectors.dense(regGradient), gradient)
+ val (regLoss, regGradient) = regFun.calculate(Vectors.fromBreeze(coefficients))
--- End diff --
When you say compose, do you just mean have the same input/output types, or do you mean you want to do something like this?:
````scala
val combinedLoss: DiffFunction[Vector] = dataLoss.add(regLoss)
````
I do agree it's awkward. Ideally, the loss functions would use Spark vectors but that isn't possible since Breeze requires that the type being optimized implements certain type classes. However, we don't directly optimize the regularization so it doesn't have to be a Breeze vector. Could we make it all Breeze? Yes, but then we'd have to convert the gradient from the aggregator to Breeze and add them using Breeze ops... or something. I don't see that one way is really better than the other, until we can implement something more comprehensive - for example, implementing optimization framework directly in Spark would/will solve this: https://issues.apache.org/jira/browse/SPARK-17136
I suppose I'm willing to change this, though I'm not sure how much it helps, and since it's all internal we aren't locking ourselves into anything. Additionally, doing these awkward conversions only happens here, in one place so it's not spread throughout the codebase or anything. Thoughts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r126582816
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregatorSuite.scala ---
@@ -0,0 +1,254 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.ml.optim.aggregator
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.ml.feature.Instance
+import org.apache.spark.ml.linalg.{BLAS, Matrices, Vector, Vectors}
+import org.apache.spark.ml.util.TestingUtils._
+import org.apache.spark.mllib.util.MLlibTestSparkContext
+
+class LogisticAggregatorSuite extends SparkFunSuite with MLlibTestSparkContext {
+
+ import DifferentiableLossAggregatorSuite.getClassificationSummarizers
+
+ @transient var instances: Array[Instance] = _
+ @transient var instancesConstantFeature: Array[Instance] = _
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ instances = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.5, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(4.0, 0.5))
+ )
+ instancesConstantFeature = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.0, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(1.0, 0.5))
+ )
+ }
+
+
+ /** Get summary statistics for some data and create a new LogisticAggregator. */
+ private def getNewAggregator(
+ instances: Array[Instance],
+ coefficients: Vector,
+ fitIntercept: Boolean,
+ isMultinomial: Boolean): LogisticAggregator = {
+ val (featuresSummarizer, ySummarizer) =
+ DifferentiableLossAggregatorSuite.getClassificationSummarizers(instances)
+ val numClasses = ySummarizer.histogram.length
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val bcFeaturesStd = spark.sparkContext.broadcast(featuresStd)
+ val bcCoefficients = spark.sparkContext.broadcast(coefficients)
--- End diff --
I think we always try to destroy broadcast variable explicitly both in source code and test cases, like [here](https://github.com/apache/spark/pull/18152). Of course, these broadcast variables can be destroyed after spark session is torn down.
The reason of why we do this in source code is users application may be long-time running, so it will accumulate lots of these variables, waste lots of resource and slower your application.
The reason of why we do this in test case is we should keep same code route as in source code. Since we have encountered similar bugs which was not covered by test cases.
But in this case, I think it's safe to not destroy these variables. I just suggested to follow MLlib's convention. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18305
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/79715/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r125759270
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregatorSuite.scala ---
@@ -0,0 +1,254 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.ml.optim.aggregator
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.ml.feature.Instance
+import org.apache.spark.ml.linalg.{BLAS, Matrices, Vector, Vectors}
+import org.apache.spark.ml.util.TestingUtils._
+import org.apache.spark.mllib.util.MLlibTestSparkContext
+
+class LogisticAggregatorSuite extends SparkFunSuite with MLlibTestSparkContext {
+
+ import DifferentiableLossAggregatorSuite.getClassificationSummarizers
+
+ @transient var instances: Array[Instance] = _
+ @transient var instancesConstantFeature: Array[Instance] = _
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ instances = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.5, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(4.0, 0.5))
+ )
+ instancesConstantFeature = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.0, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(1.0, 0.5))
+ )
+ }
+
+
+ /** Get summary statistics for some data and create a new LogisticAggregator. */
+ private def getNewAggregator(
+ instances: Array[Instance],
+ coefficients: Vector,
+ fitIntercept: Boolean,
+ isMultinomial: Boolean): LogisticAggregator = {
+ val (featuresSummarizer, ySummarizer) =
+ DifferentiableLossAggregatorSuite.getClassificationSummarizers(instances)
+ val numClasses = ySummarizer.histogram.length
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val bcFeaturesStd = spark.sparkContext.broadcast(featuresStd)
+ val bcCoefficients = spark.sparkContext.broadcast(coefficients)
--- End diff --
Thanks. I added some code to do this, it's certainly clunky, but should suffice.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by hhbyyh <gi...@git.apache.org>.
Github user hhbyyh commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r124138183
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/loss/RDDLossFunction.scala ---
@@ -50,7 +50,7 @@ private[ml] class RDDLossFunction[
Agg <: DifferentiableLossAggregator[T, Agg]: ClassTag](
instances: RDD[T],
getAggregator: (Broadcast[Vector] => Agg),
- regularization: Option[DifferentiableRegularization[Array[Double]]],
+ regularization: Option[DifferentiableRegularization[Vector]],
--- End diff --
support sparse coefficients I guess.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18305
**[Test build #78063 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78063/testReport)** for PR 18305 at commit [`6edd128`](https://github.com/apache/spark/commit/6edd12893a255c1f44416a16e42c6ef79edc8f36).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r125757263
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/loss/RDDLossFunction.scala ---
@@ -62,8 +62,8 @@ private[ml] class RDDLossFunction[
val newAgg = instances.treeAggregate(thisAgg)(seqOp, combOp, aggregationDepth)
val gradient = newAgg.gradient
val regLoss = regularization.map { regFun =>
- val (regLoss, regGradient) = regFun.calculate(coefficients.data)
- BLAS.axpy(1.0, Vectors.dense(regGradient), gradient)
+ val (regLoss, regGradient) = regFun.calculate(Vectors.fromBreeze(coefficients))
--- End diff --
I agree there is room for improvement. My preference is to leave it for a follow up, though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by hhbyyh <gi...@git.apache.org>.
Github user hhbyyh commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r124135824
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/loss/DifferentiableRegularization.scala ---
@@ -38,34 +40,39 @@ private[ml] trait DifferentiableRegularization[T] extends DiffFunction[T] {
* @param regParam The magnitude of the regularization.
* @param shouldApply A function (Int => Boolean) indicating whether a given index should have
* regularization applied to it.
- * @param featuresStd Option indicating whether the regularization should be scaled by the standard
- * deviation of the features.
+ * @param featuresStd Option for a function which maps coefficient index (column major) to the
+ * feature standard deviation. If `None`, no standardization is applied.
*/
private[ml] class L2Regularization(
- val regParam: Double,
+ override val regParam: Double,
shouldApply: Int => Boolean,
- featuresStd: Option[Array[Double]]) extends DifferentiableRegularization[Array[Double]] {
+ featuresStd: Option[Int => Double]) extends DifferentiableRegularization[Vector] {
- override def calculate(coefficients: Array[Double]): (Double, Array[Double]) = {
- var sum = 0.0
- val gradient = new Array[Double](coefficients.length)
- coefficients.indices.filter(shouldApply).foreach { j =>
- val coef = coefficients(j)
- featuresStd match {
- case Some(stds) =>
- val std = stds(j)
- if (std != 0.0) {
- val temp = coef / (std * std)
- sum += coef * temp
- gradient(j) = regParam * temp
- } else {
- 0.0
+ override def calculate(coefficients: Vector): (Double, Vector) = {
+ coefficients match {
+ case dv: DenseVector =>
+ var sum = 0.0
+ val gradient = new Array[Double](dv.size)
+ dv.values.indices.filter(shouldApply).foreach { j =>
+ val coef = coefficients(j)
+ featuresStd match {
+ case Some(getStd) =>
+ val std = getStd(j)
+ if (std != 0.0) {
+ val temp = coef / (std * std)
+ sum += coef * temp
+ gradient(j) = regParam * temp
+ } else {
+ 0.0
+ }
+ case None =>
+ sum += coef * coef
+ gradient(j) = coef * regParam
}
- case None =>
- sum += coef * coef
- gradient(j) = coef * regParam
- }
+ }
+ (0.5 * sum * regParam, Vectors.dense(gradient))
+ case _: SparseVector =>
+ throw new IllegalArgumentException("SparseVector is not currently supported.")
--- End diff --
message: Sparse coefficients...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r124238951
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/DifferentiableLossAggregatorSuite.scala ---
@@ -157,4 +160,38 @@ object DifferentiableLossAggregatorSuite {
this
}
}
+
+ /** Get feature and label summarizers for provided data. */
+ private[ml] def getRegressionSummarizers(
--- End diff --
Seems like these convenience functions could be adapted to also be used in both in `train` logic across the linear models as well as here in the tests? Since the `seqOp` and `comboOp` are defined multiple times everywhere.
Though that could be done in a later clean up PR perhaps?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on the issue:
https://github.com/apache/spark/pull/18305
Thanks @MLnick, @hhbyyh, and @facaiy for reviewing!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on the issue:
https://github.com/apache/spark/pull/18305
Merged to master. Thanks @sethah, and thanks all for reviews.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18305
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r124238050
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/loss/RDDLossFunction.scala ---
@@ -62,8 +62,8 @@ private[ml] class RDDLossFunction[
val newAgg = instances.treeAggregate(thisAgg)(seqOp, combOp, aggregationDepth)
val gradient = newAgg.gradient
val regLoss = regularization.map { regFun =>
- val (regLoss, regGradient) = regFun.calculate(coefficients.data)
- BLAS.axpy(1.0, Vectors.dense(regGradient), gradient)
+ val (regLoss, regGradient) = regFun.calculate(Vectors.fromBreeze(coefficients))
--- End diff --
So here, for example, we need to convert from Breeze. We could just make the L2 reg also for Breeze vector? It feels like we should just be able to compose the loss function and reg loss function together.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18305
Merged build finished. Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r124614829
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregatorSuite.scala ---
@@ -0,0 +1,254 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.ml.optim.aggregator
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.ml.feature.Instance
+import org.apache.spark.ml.linalg.{BLAS, Matrices, Vector, Vectors}
+import org.apache.spark.ml.util.TestingUtils._
+import org.apache.spark.mllib.util.MLlibTestSparkContext
+
+class LogisticAggregatorSuite extends SparkFunSuite with MLlibTestSparkContext {
+
+ import DifferentiableLossAggregatorSuite.getClassificationSummarizers
+
+ @transient var instances: Array[Instance] = _
+ @transient var instancesConstantFeature: Array[Instance] = _
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ instances = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.5, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(4.0, 0.5))
+ )
+ instancesConstantFeature = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.0, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(1.0, 0.5))
+ )
+ }
+
+
+ /** Get summary statistics for some data and create a new LogisticAggregator. */
+ private def getNewAggregator(
+ instances: Array[Instance],
+ coefficients: Vector,
+ fitIntercept: Boolean,
+ isMultinomial: Boolean): LogisticAggregator = {
+ val (featuresSummarizer, ySummarizer) =
+ DifferentiableLossAggregatorSuite.getClassificationSummarizers(instances)
+ val numClasses = ySummarizer.histogram.length
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val bcFeaturesStd = spark.sparkContext.broadcast(featuresStd)
+ val bcCoefficients = spark.sparkContext.broadcast(coefficients)
+ new LogisticAggregator(bcFeaturesStd, numClasses, fitIntercept, isMultinomial)(bcCoefficients)
+ }
+
+ test("aggregator add method input size") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator features dimension must match coefficients dimension") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, 1.0, Vectors.dense(2.0)))
+ }
+ }
+ }
+
+ test("negative weight") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator does not support negative instance weights") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, -1.0, Vectors.dense(2.0, 1.0)))
+ }
+ }
+ }
+
+ test("check sizes multinomial") {
+ val rng = new scala.util.Random
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val coefWithIntercept = Vectors.dense(
+ Array.fill(numClasses * (numFeatures + 1))(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(
+ Array.fill(numClasses * numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(instances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = true)
+ val aggNoIntercept = getNewAggregator(instances, coefWithoutIntercept, fitIntercept = false,
+ isMultinomial = true)
+ instances.foreach(aggIntercept.add)
+ instances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === (numFeatures + 1) * numClasses)
+ assert(aggNoIntercept.gradient.size === numFeatures * numClasses)
+ }
+
+ test("check sizes binomial") {
+ val rng = new scala.util.Random
+ val binaryInstances = instances.filter(_.label < 2.0)
+ val numFeatures = binaryInstances.head.features.size
+ val coefWithIntercept = Vectors.dense(Array.fill(numFeatures + 1)(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(Array.fill(numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(binaryInstances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = false)
+ val aggNoIntercept = getNewAggregator(binaryInstances, coefWithoutIntercept,
+ fitIntercept = false, isMultinomial = false)
+ binaryInstances.foreach(aggIntercept.add)
+ binaryInstances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === numFeatures + 1)
+ assert(aggNoIntercept.gradient.size === numFeatures)
+ }
+
+ test("check correctness multinomial") {
+ /*
+ Check that the aggregator computes loss/gradient for:
+ -sum_i w_i * (beta_y dot x_i - log(sum_k e^(beta_k dot x_i)))
+ */
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val intercepts = Vectors.dense(interceptArray)
+ val (featuresSummarizer, ySummarizer) = getClassificationSummarizers(instances)
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val weightSum = instances.map(_.weight).sum
+
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ instances.foreach(agg.add)
+
+ // compute the loss
+ val stdCoef = coefArray.indices.map(i => coefArray(i) / featuresStd(i / numClasses)).toArray
--- End diff --
Not sure I follow. What path do you mean?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18305
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on the issue:
https://github.com/apache/spark/pull/18305
I get it in source code. I'm indifferent in tests. If it's easy sure.
How does it help uncover bugs though?
On Tue, 11 Jul 2017 at 04:42, Yanbo Liang <no...@github.com> wrote:
> *@yanboliang* commented on this pull request.
> ------------------------------
>
> In
> mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregatorSuite.scala
> <https://github.com/apache/spark/pull/18305#discussion_r126582816>:
>
> > + )
> + }
> +
> +
> + /** Get summary statistics for some data and create a new LogisticAggregator. */
> + private def getNewAggregator(
> + instances: Array[Instance],
> + coefficients: Vector,
> + fitIntercept: Boolean,
> + isMultinomial: Boolean): LogisticAggregator = {
> + val (featuresSummarizer, ySummarizer) =
> + DifferentiableLossAggregatorSuite.getClassificationSummarizers(instances)
> + val numClasses = ySummarizer.histogram.length
> + val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
> + val bcFeaturesStd = spark.sparkContext.broadcast(featuresStd)
> + val bcCoefficients = spark.sparkContext.broadcast(coefficients)
>
> I think we always try to destroy broadcast variable explicitly both in
> source code and test cases, like here
> <https://github.com/apache/spark/pull/18152>. Of course, these broadcast
> variables can be destroyed after spark session is torn down.
> The reason of why we do this in source code is users application may be
> long-time running, so it will accumulate lots of these variables, waste
> lots of resource and slower your application.
> The reason of why we do this in test case is we should keep same code
> route as in source code. Since we have encountered similar bugs which was
> not covered by test cases.
> But in this case, I think it's safe to not destroy these variables. I just
> suggested to follow MLlib's convention. Thanks.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/spark/pull/18305#discussion_r126582816>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AA_SB6vnRqfFLxdk23k9Q51rWhMu0KgCks5sMuEdgaJpZM4N6TDe>
> .
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r125024716
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregatorSuite.scala ---
@@ -0,0 +1,254 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.ml.optim.aggregator
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.ml.feature.Instance
+import org.apache.spark.ml.linalg.{BLAS, Matrices, Vector, Vectors}
+import org.apache.spark.ml.util.TestingUtils._
+import org.apache.spark.mllib.util.MLlibTestSparkContext
+
+class LogisticAggregatorSuite extends SparkFunSuite with MLlibTestSparkContext {
+
+ import DifferentiableLossAggregatorSuite.getClassificationSummarizers
+
+ @transient var instances: Array[Instance] = _
+ @transient var instancesConstantFeature: Array[Instance] = _
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ instances = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.5, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(4.0, 0.5))
+ )
+ instancesConstantFeature = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.0, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(1.0, 0.5))
+ )
+ }
+
+
+ /** Get summary statistics for some data and create a new LogisticAggregator. */
+ private def getNewAggregator(
+ instances: Array[Instance],
+ coefficients: Vector,
+ fitIntercept: Boolean,
+ isMultinomial: Boolean): LogisticAggregator = {
+ val (featuresSummarizer, ySummarizer) =
+ DifferentiableLossAggregatorSuite.getClassificationSummarizers(instances)
+ val numClasses = ySummarizer.histogram.length
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val bcFeaturesStd = spark.sparkContext.broadcast(featuresStd)
+ val bcCoefficients = spark.sparkContext.broadcast(coefficients)
+ new LogisticAggregator(bcFeaturesStd, numClasses, fitIntercept, isMultinomial)(bcCoefficients)
+ }
+
+ test("aggregator add method input size") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator features dimension must match coefficients dimension") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, 1.0, Vectors.dense(2.0)))
+ }
+ }
+ }
+
+ test("negative weight") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator does not support negative instance weights") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, -1.0, Vectors.dense(2.0, 1.0)))
+ }
+ }
+ }
+
+ test("check sizes multinomial") {
+ val rng = new scala.util.Random
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val coefWithIntercept = Vectors.dense(
+ Array.fill(numClasses * (numFeatures + 1))(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(
+ Array.fill(numClasses * numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(instances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = true)
+ val aggNoIntercept = getNewAggregator(instances, coefWithoutIntercept, fitIntercept = false,
+ isMultinomial = true)
+ instances.foreach(aggIntercept.add)
+ instances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === (numFeatures + 1) * numClasses)
+ assert(aggNoIntercept.gradient.size === numFeatures * numClasses)
+ }
+
+ test("check sizes binomial") {
+ val rng = new scala.util.Random
+ val binaryInstances = instances.filter(_.label < 2.0)
+ val numFeatures = binaryInstances.head.features.size
+ val coefWithIntercept = Vectors.dense(Array.fill(numFeatures + 1)(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(Array.fill(numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(binaryInstances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = false)
+ val aggNoIntercept = getNewAggregator(binaryInstances, coefWithoutIntercept,
+ fitIntercept = false, isMultinomial = false)
+ binaryInstances.foreach(aggIntercept.add)
+ binaryInstances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === numFeatures + 1)
+ assert(aggNoIntercept.gradient.size === numFeatures)
+ }
+
+ test("check correctness multinomial") {
+ /*
+ Check that the aggregator computes loss/gradient for:
+ -sum_i w_i * (beta_y dot x_i - log(sum_k e^(beta_k dot x_i)))
+ */
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val intercepts = Vectors.dense(interceptArray)
+ val (featuresSummarizer, ySummarizer) = getClassificationSummarizers(instances)
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val weightSum = instances.map(_.weight).sum
+
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ instances.foreach(agg.add)
+
+ // compute the loss
+ val stdCoef = coefArray.indices.map(i => coefArray(i) / featuresStd(i / numClasses)).toArray
--- End diff --
Here we're checking that the loss and gradient computed analytically are equivalent to that computed by the aggregator. Here we check the "standardized" version (that does implicit on the fly standardization). But doesn't that mean we haven't compared the `standardization=false` case?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r127064727
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregatorSuite.scala ---
@@ -0,0 +1,254 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.ml.optim.aggregator
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.ml.feature.Instance
+import org.apache.spark.ml.linalg.{BLAS, Matrices, Vector, Vectors}
+import org.apache.spark.ml.util.TestingUtils._
+import org.apache.spark.mllib.util.MLlibTestSparkContext
+
+class LogisticAggregatorSuite extends SparkFunSuite with MLlibTestSparkContext {
+
+ import DifferentiableLossAggregatorSuite.getClassificationSummarizers
+
+ @transient var instances: Array[Instance] = _
+ @transient var instancesConstantFeature: Array[Instance] = _
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ instances = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.5, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(4.0, 0.5))
+ )
+ instancesConstantFeature = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.0, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(1.0, 0.5))
+ )
+ }
+
+
+ /** Get summary statistics for some data and create a new LogisticAggregator. */
+ private def getNewAggregator(
+ instances: Array[Instance],
+ coefficients: Vector,
+ fitIntercept: Boolean,
+ isMultinomial: Boolean): LogisticAggregator = {
+ val (featuresSummarizer, ySummarizer) =
+ DifferentiableLossAggregatorSuite.getClassificationSummarizers(instances)
+ val numClasses = ySummarizer.histogram.length
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val bcFeaturesStd = spark.sparkContext.broadcast(featuresStd)
+ val bcCoefficients = spark.sparkContext.broadcast(coefficients)
+ new LogisticAggregator(bcFeaturesStd, numClasses, fitIntercept, isMultinomial)(bcCoefficients)
+ }
+
+ test("aggregator add method input size") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator features dimension must match coefficients dimension") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, 1.0, Vectors.dense(2.0)))
+ }
+ }
+ }
+
+ test("negative weight") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator does not support negative instance weights") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, -1.0, Vectors.dense(2.0, 1.0)))
+ }
+ }
+ }
+
+ test("check sizes multinomial") {
+ val rng = new scala.util.Random
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val coefWithIntercept = Vectors.dense(
+ Array.fill(numClasses * (numFeatures + 1))(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(
+ Array.fill(numClasses * numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(instances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = true)
+ val aggNoIntercept = getNewAggregator(instances, coefWithoutIntercept, fitIntercept = false,
+ isMultinomial = true)
+ instances.foreach(aggIntercept.add)
+ instances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === (numFeatures + 1) * numClasses)
+ assert(aggNoIntercept.gradient.size === numFeatures * numClasses)
+ }
+
+ test("check sizes binomial") {
+ val rng = new scala.util.Random
+ val binaryInstances = instances.filter(_.label < 2.0)
+ val numFeatures = binaryInstances.head.features.size
+ val coefWithIntercept = Vectors.dense(Array.fill(numFeatures + 1)(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(Array.fill(numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(binaryInstances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = false)
+ val aggNoIntercept = getNewAggregator(binaryInstances, coefWithoutIntercept,
+ fitIntercept = false, isMultinomial = false)
+ binaryInstances.foreach(aggIntercept.add)
+ binaryInstances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === numFeatures + 1)
+ assert(aggNoIntercept.gradient.size === numFeatures)
+ }
+
+ test("check correctness multinomial") {
+ /*
+ Check that the aggregator computes loss/gradient for:
+ -sum_i w_i * (beta_y dot x_i - log(sum_k e^(beta_k dot x_i)))
+ */
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val intercepts = Vectors.dense(interceptArray)
+ val (featuresSummarizer, ySummarizer) = getClassificationSummarizers(instances)
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val weightSum = instances.map(_.weight).sum
+
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ instances.foreach(agg.add)
+
+ // compute the loss
+ val stdCoef = coefArray.indices.map(i => coefArray(i) / featuresStd(i / numClasses)).toArray
--- End diff --
Yes this is the intended behavior. You can read the scaladoc to verify as such.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on the issue:
https://github.com/apache/spark/pull/18305
@MLnick I remembered a bug I hit several months ago: we forgot to destroy a broadcast variable in source code, but it throws exception after we add destroy explicitly. This is because we put broadcast variable in wrong place, it works well if we don't destroy it explicitly. This could be avoid if we add corresponding test cases with destroy explicitly. If not, we may forgot to destroy broadcast variable explicitly in source code, which is likely to lead to potential bugs. This is why I'm prefer to keep consistent logic in both source and test. I think it's trivial issue for this case. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18305
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/79244/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18305
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/78065/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r127935551
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregatorSuite.scala ---
@@ -0,0 +1,254 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.ml.optim.aggregator
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.ml.feature.Instance
+import org.apache.spark.ml.linalg.{BLAS, Matrices, Vector, Vectors}
+import org.apache.spark.ml.util.TestingUtils._
+import org.apache.spark.mllib.util.MLlibTestSparkContext
+
+class LogisticAggregatorSuite extends SparkFunSuite with MLlibTestSparkContext {
+
+ import DifferentiableLossAggregatorSuite.getClassificationSummarizers
+
+ @transient var instances: Array[Instance] = _
+ @transient var instancesConstantFeature: Array[Instance] = _
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ instances = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.5, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(4.0, 0.5))
+ )
+ instancesConstantFeature = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.0, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(1.0, 0.5))
+ )
+ }
+
+
+ /** Get summary statistics for some data and create a new LogisticAggregator. */
+ private def getNewAggregator(
+ instances: Array[Instance],
+ coefficients: Vector,
+ fitIntercept: Boolean,
+ isMultinomial: Boolean): LogisticAggregator = {
+ val (featuresSummarizer, ySummarizer) =
+ DifferentiableLossAggregatorSuite.getClassificationSummarizers(instances)
+ val numClasses = ySummarizer.histogram.length
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val bcFeaturesStd = spark.sparkContext.broadcast(featuresStd)
+ val bcCoefficients = spark.sparkContext.broadcast(coefficients)
+ new LogisticAggregator(bcFeaturesStd, numClasses, fitIntercept, isMultinomial)(bcCoefficients)
+ }
+
+ test("aggregator add method input size") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator features dimension must match coefficients dimension") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, 1.0, Vectors.dense(2.0)))
+ }
+ }
+ }
+
+ test("negative weight") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator does not support negative instance weights") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, -1.0, Vectors.dense(2.0, 1.0)))
+ }
+ }
+ }
+
+ test("check sizes multinomial") {
+ val rng = new scala.util.Random
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val coefWithIntercept = Vectors.dense(
+ Array.fill(numClasses * (numFeatures + 1))(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(
+ Array.fill(numClasses * numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(instances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = true)
+ val aggNoIntercept = getNewAggregator(instances, coefWithoutIntercept, fitIntercept = false,
+ isMultinomial = true)
+ instances.foreach(aggIntercept.add)
+ instances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === (numFeatures + 1) * numClasses)
+ assert(aggNoIntercept.gradient.size === numFeatures * numClasses)
+ }
+
+ test("check sizes binomial") {
+ val rng = new scala.util.Random
+ val binaryInstances = instances.filter(_.label < 2.0)
+ val numFeatures = binaryInstances.head.features.size
+ val coefWithIntercept = Vectors.dense(Array.fill(numFeatures + 1)(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(Array.fill(numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(binaryInstances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = false)
+ val aggNoIntercept = getNewAggregator(binaryInstances, coefWithoutIntercept,
+ fitIntercept = false, isMultinomial = false)
+ binaryInstances.foreach(aggIntercept.add)
+ binaryInstances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === numFeatures + 1)
+ assert(aggNoIntercept.gradient.size === numFeatures)
+ }
+
+ test("check correctness multinomial") {
+ /*
+ Check that the aggregator computes loss/gradient for:
+ -sum_i w_i * (beta_y dot x_i - log(sum_k e^(beta_k dot x_i)))
+ */
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val intercepts = Vectors.dense(interceptArray)
+ val (featuresSummarizer, ySummarizer) = getClassificationSummarizers(instances)
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val weightSum = instances.map(_.weight).sum
+
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ instances.foreach(agg.add)
+
+ // compute the loss
+ val stdCoef = coefArray.indices.map(i => coefArray(i) / featuresStd(i / numClasses)).toArray
--- End diff --
I know it's intended. But to me it's not _expected_ as a user. Surely setting `standardization=false`, a user expects the model **not** to be trained in the scaled space?
Put another way - what's the point of having the regularization loss effectively not scaled, but the objective loss always scaled?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r125681761
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/loss/DifferentiableRegularization.scala ---
@@ -32,40 +34,45 @@ private[ml] trait DifferentiableRegularization[T] extends DiffFunction[T] {
}
/**
- * A Breeze diff function for computing the L2 regularized loss and gradient of an array of
+ * A Breeze diff function for computing the L2 regularized loss and gradient of a vector of
* coefficients.
*
* @param regParam The magnitude of the regularization.
* @param shouldApply A function (Int => Boolean) indicating whether a given index should have
* regularization applied to it.
- * @param featuresStd Option indicating whether the regularization should be scaled by the standard
- * deviation of the features.
+ * @param featuresStd Option for a function which maps coefficient index (column major) to the
--- End diff --
done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r125675879
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregatorSuite.scala ---
@@ -0,0 +1,254 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.ml.optim.aggregator
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.ml.feature.Instance
+import org.apache.spark.ml.linalg.{BLAS, Matrices, Vector, Vectors}
+import org.apache.spark.ml.util.TestingUtils._
+import org.apache.spark.mllib.util.MLlibTestSparkContext
+
+class LogisticAggregatorSuite extends SparkFunSuite with MLlibTestSparkContext {
+
+ import DifferentiableLossAggregatorSuite.getClassificationSummarizers
+
+ @transient var instances: Array[Instance] = _
+ @transient var instancesConstantFeature: Array[Instance] = _
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ instances = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.5, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(4.0, 0.5))
+ )
+ instancesConstantFeature = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.0, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(1.0, 0.5))
+ )
+ }
+
+
+ /** Get summary statistics for some data and create a new LogisticAggregator. */
+ private def getNewAggregator(
+ instances: Array[Instance],
+ coefficients: Vector,
+ fitIntercept: Boolean,
+ isMultinomial: Boolean): LogisticAggregator = {
+ val (featuresSummarizer, ySummarizer) =
+ DifferentiableLossAggregatorSuite.getClassificationSummarizers(instances)
+ val numClasses = ySummarizer.histogram.length
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val bcFeaturesStd = spark.sparkContext.broadcast(featuresStd)
+ val bcCoefficients = spark.sparkContext.broadcast(coefficients)
+ new LogisticAggregator(bcFeaturesStd, numClasses, fitIntercept, isMultinomial)(bcCoefficients)
+ }
+
+ test("aggregator add method input size") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator features dimension must match coefficients dimension") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, 1.0, Vectors.dense(2.0)))
+ }
+ }
+ }
+
+ test("negative weight") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator does not support negative instance weights") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, -1.0, Vectors.dense(2.0, 1.0)))
+ }
+ }
+ }
+
+ test("check sizes multinomial") {
+ val rng = new scala.util.Random
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val coefWithIntercept = Vectors.dense(
+ Array.fill(numClasses * (numFeatures + 1))(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(
+ Array.fill(numClasses * numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(instances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = true)
+ val aggNoIntercept = getNewAggregator(instances, coefWithoutIntercept, fitIntercept = false,
+ isMultinomial = true)
+ instances.foreach(aggIntercept.add)
+ instances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === (numFeatures + 1) * numClasses)
+ assert(aggNoIntercept.gradient.size === numFeatures * numClasses)
+ }
+
+ test("check sizes binomial") {
+ val rng = new scala.util.Random
+ val binaryInstances = instances.filter(_.label < 2.0)
+ val numFeatures = binaryInstances.head.features.size
+ val coefWithIntercept = Vectors.dense(Array.fill(numFeatures + 1)(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(Array.fill(numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(binaryInstances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = false)
+ val aggNoIntercept = getNewAggregator(binaryInstances, coefWithoutIntercept,
+ fitIntercept = false, isMultinomial = false)
+ binaryInstances.foreach(aggIntercept.add)
+ binaryInstances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === numFeatures + 1)
+ assert(aggNoIntercept.gradient.size === numFeatures)
+ }
+
+ test("check correctness multinomial") {
+ /*
+ Check that the aggregator computes loss/gradient for:
+ -sum_i w_i * (beta_y dot x_i - log(sum_k e^(beta_k dot x_i)))
+ */
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val intercepts = Vectors.dense(interceptArray)
+ val (featuresSummarizer, ySummarizer) = getClassificationSummarizers(instances)
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val weightSum = instances.map(_.weight).sum
+
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ instances.foreach(agg.add)
+
+ // compute the loss
+ val stdCoef = coefArray.indices.map(i => coefArray(i) / featuresStd(i / numClasses)).toArray
--- End diff --
It's better to check the ```standardization=false```.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18305
**[Test build #78820 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78820/testReport)** for PR 18305 at commit [`a19b385`](https://github.com/apache/spark/commit/a19b385913f450254b6849098f68cab4c2dac618).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r124614166
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/DifferentiableLossAggregatorSuite.scala ---
@@ -157,4 +160,38 @@ object DifferentiableLossAggregatorSuite {
this
}
}
+
+ /** Get feature and label summarizers for provided data. */
+ private[ml] def getRegressionSummarizers(
--- End diff --
Yeah, I thought about it. Best for a follow up I think.
https://issues.apache.org/jira/browse/SPARK-21245
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on the issue:
https://github.com/apache/spark/pull/18305
Thanks @MLnick. I agree with you about the broadcasting, so have backed it out. I think all comments are addressed now, please let me know if there is anything else.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r124615112
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/loss/RDDLossFunction.scala ---
@@ -50,7 +50,7 @@ private[ml] class RDDLossFunction[
Agg <: DifferentiableLossAggregator[T, Agg]: ClassTag](
instances: RDD[T],
getAggregator: (Broadcast[Vector] => Agg),
- regularization: Option[DifferentiableRegularization[Array[Double]]],
+ regularization: Option[DifferentiableRegularization[Vector]],
--- End diff --
Commented on this below.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r124615145
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/loss/DifferentiableRegularization.scala ---
@@ -38,34 +40,39 @@ private[ml] trait DifferentiableRegularization[T] extends DiffFunction[T] {
* @param regParam The magnitude of the regularization.
* @param shouldApply A function (Int => Boolean) indicating whether a given index should have
* regularization applied to it.
- * @param featuresStd Option indicating whether the regularization should be scaled by the standard
- * deviation of the features.
+ * @param featuresStd Option for a function which maps coefficient index (column major) to the
+ * feature standard deviation. If `None`, no standardization is applied.
*/
private[ml] class L2Regularization(
- val regParam: Double,
+ override val regParam: Double,
shouldApply: Int => Boolean,
- featuresStd: Option[Array[Double]]) extends DifferentiableRegularization[Array[Double]] {
+ featuresStd: Option[Int => Double]) extends DifferentiableRegularization[Vector] {
- override def calculate(coefficients: Array[Double]): (Double, Array[Double]) = {
- var sum = 0.0
- val gradient = new Array[Double](coefficients.length)
- coefficients.indices.filter(shouldApply).foreach { j =>
- val coef = coefficients(j)
- featuresStd match {
- case Some(stds) =>
- val std = stds(j)
- if (std != 0.0) {
- val temp = coef / (std * std)
- sum += coef * temp
- gradient(j) = regParam * temp
- } else {
- 0.0
+ override def calculate(coefficients: Vector): (Double, Vector) = {
+ coefficients match {
+ case dv: DenseVector =>
+ var sum = 0.0
+ val gradient = new Array[Double](dv.size)
+ dv.values.indices.filter(shouldApply).foreach { j =>
+ val coef = coefficients(j)
+ featuresStd match {
+ case Some(getStd) =>
+ val std = getStd(j)
+ if (std != 0.0) {
+ val temp = coef / (std * std)
+ sum += coef * temp
+ gradient(j) = regParam * temp
+ } else {
+ 0.0
+ }
+ case None =>
+ sum += coef * coef
+ gradient(j) = coef * regParam
}
- case None =>
- sum += coef * coef
- gradient(j) = coef * regParam
- }
+ }
+ (0.5 * sum * regParam, Vectors.dense(gradient))
+ case _: SparseVector =>
+ throw new IllegalArgumentException("SparseVector is not currently supported.")
--- End diff --
done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r124615494
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/loss/DifferentiableRegularization.scala ---
@@ -38,34 +40,39 @@ private[ml] trait DifferentiableRegularization[T] extends DiffFunction[T] {
* @param regParam The magnitude of the regularization.
--- End diff --
Good catch. Done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by yanboliang <gi...@git.apache.org>.
Github user yanboliang commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r125671626
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/loss/DifferentiableRegularization.scala ---
@@ -32,40 +34,45 @@ private[ml] trait DifferentiableRegularization[T] extends DiffFunction[T] {
}
/**
- * A Breeze diff function for computing the L2 regularized loss and gradient of an array of
+ * A Breeze diff function for computing the L2 regularized loss and gradient of a vector of
* coefficients.
*
* @param regParam The magnitude of the regularization.
* @param shouldApply A function (Int => Boolean) indicating whether a given index should have
* regularization applied to it.
- * @param featuresStd Option indicating whether the regularization should be scaled by the standard
- * deviation of the features.
+ * @param featuresStd Option for a function which maps coefficient index (column major) to the
--- End diff --
Should ```getFeaturesStd``` or ```applyFeaturesStd``` be better? Since this is a function, not the original features std array.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18305
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18305
**[Test build #79715 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/79715/testReport)** for PR 18305 at commit [`cb40b31`](https://github.com/apache/spark/commit/cb40b3189b546283635683d7711e0e2e28366034).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18305
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r128041731
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregatorSuite.scala ---
@@ -0,0 +1,254 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.ml.optim.aggregator
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.ml.feature.Instance
+import org.apache.spark.ml.linalg.{BLAS, Matrices, Vector, Vectors}
+import org.apache.spark.ml.util.TestingUtils._
+import org.apache.spark.mllib.util.MLlibTestSparkContext
+
+class LogisticAggregatorSuite extends SparkFunSuite with MLlibTestSparkContext {
+
+ import DifferentiableLossAggregatorSuite.getClassificationSummarizers
+
+ @transient var instances: Array[Instance] = _
+ @transient var instancesConstantFeature: Array[Instance] = _
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ instances = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.5, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(4.0, 0.5))
+ )
+ instancesConstantFeature = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.0, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(1.0, 0.5))
+ )
+ }
+
+
+ /** Get summary statistics for some data and create a new LogisticAggregator. */
+ private def getNewAggregator(
+ instances: Array[Instance],
+ coefficients: Vector,
+ fitIntercept: Boolean,
+ isMultinomial: Boolean): LogisticAggregator = {
+ val (featuresSummarizer, ySummarizer) =
+ DifferentiableLossAggregatorSuite.getClassificationSummarizers(instances)
+ val numClasses = ySummarizer.histogram.length
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val bcFeaturesStd = spark.sparkContext.broadcast(featuresStd)
+ val bcCoefficients = spark.sparkContext.broadcast(coefficients)
+ new LogisticAggregator(bcFeaturesStd, numClasses, fitIntercept, isMultinomial)(bcCoefficients)
+ }
+
+ test("aggregator add method input size") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator features dimension must match coefficients dimension") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, 1.0, Vectors.dense(2.0)))
+ }
+ }
+ }
+
+ test("negative weight") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator does not support negative instance weights") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, -1.0, Vectors.dense(2.0, 1.0)))
+ }
+ }
+ }
+
+ test("check sizes multinomial") {
+ val rng = new scala.util.Random
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val coefWithIntercept = Vectors.dense(
+ Array.fill(numClasses * (numFeatures + 1))(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(
+ Array.fill(numClasses * numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(instances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = true)
+ val aggNoIntercept = getNewAggregator(instances, coefWithoutIntercept, fitIntercept = false,
+ isMultinomial = true)
+ instances.foreach(aggIntercept.add)
+ instances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === (numFeatures + 1) * numClasses)
+ assert(aggNoIntercept.gradient.size === numFeatures * numClasses)
+ }
+
+ test("check sizes binomial") {
+ val rng = new scala.util.Random
+ val binaryInstances = instances.filter(_.label < 2.0)
+ val numFeatures = binaryInstances.head.features.size
+ val coefWithIntercept = Vectors.dense(Array.fill(numFeatures + 1)(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(Array.fill(numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(binaryInstances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = false)
+ val aggNoIntercept = getNewAggregator(binaryInstances, coefWithoutIntercept,
+ fitIntercept = false, isMultinomial = false)
+ binaryInstances.foreach(aggIntercept.add)
+ binaryInstances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === numFeatures + 1)
+ assert(aggNoIntercept.gradient.size === numFeatures)
+ }
+
+ test("check correctness multinomial") {
+ /*
+ Check that the aggregator computes loss/gradient for:
+ -sum_i w_i * (beta_y dot x_i - log(sum_k e^(beta_k dot x_i)))
+ */
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val intercepts = Vectors.dense(interceptArray)
+ val (featuresSummarizer, ySummarizer) = getClassificationSummarizers(instances)
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val weightSum = instances.map(_.weight).sum
+
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ instances.foreach(agg.add)
+
+ // compute the loss
+ val stdCoef = coefArray.indices.map(i => coefArray(i) / featuresStd(i / numClasses)).toArray
--- End diff --
What users interpret `standardization` to mean may be different than what it means in Spark, sure. I wasn't around when that design choice was made.
Standardization is **always** done internally because it improves convergence. When no regularization is applied, you can verify that the two scenarios:
* training on scaled features and then converting the coefficients to the unscaled space
* training on un-scaled features
produce _the same coefficients_. So, instead of literally not standardizing the features when standardization is false, we just do it anyway because it's better for convergence, and it doesn't affect the results.
However, when regularization _is_ applied, the results are not the same. The l2 loss, for example, is `sum beta_j^2`. But the coefficients that we are using during training are scaled coefficients (since we always scale), which are effectively `beta_{j, scaled} = beta_j * sigma_j`. We need to "unstandardize" the coefficients for the regularization part of the loss because their scales do matter. If we didn't unstandardize them, we'd change the regularization loss to `sum (beta_j * sigma_j)^2`, which would not be correct if the user wished not to train in the scaled space.
Do I think this is all a bit confusing, especially for users? Yes. I believe this was done to match glmnet. I think it's fine the way it is, but that discussion is for another JIRA anyway.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r124237868
--- Diff: mllib/src/main/scala/org/apache/spark/ml/optim/loss/RDDLossFunction.scala ---
@@ -50,7 +50,7 @@ private[ml] class RDDLossFunction[
Agg <: DifferentiableLossAggregator[T, Agg]: ClassTag](
instances: RDD[T],
getAggregator: (Broadcast[Vector] => Agg),
- regularization: Option[DifferentiableRegularization[Array[Double]]],
+ regularization: Option[DifferentiableRegularization[Vector]],
--- End diff --
Sure. But it seems a little strange to make this a Spark `Vector` while `RDDLossFunction` is for a Breeze `Vector`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on the issue:
https://github.com/apache/spark/pull/18305
also ping @hhbyyh @yanboliang This is a straightforward follow up to https://github.com/apache/spark/pull/17094. Let me know if I can do anything to make the review easier.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18305
**[Test build #78063 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78063/testReport)** for PR 18305 at commit [`6edd128`](https://github.com/apache/spark/commit/6edd12893a255c1f44416a16e42c6ef79edc8f36).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18305: [SPARK-20988][ML] Logistic regression uses aggregator hi...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on the issue:
https://github.com/apache/spark/pull/18305
No blockers, I'm pretty happy. I just wanted to do a final pass mostly over the test cases.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r127873828
--- Diff: mllib/src/main/scala/org/apache/spark/ml/classification/LogisticRegression.scala ---
@@ -598,8 +598,23 @@ class LogisticRegression @Since("1.2.0") (
val regParamL2 = (1.0 - $(elasticNetParam)) * $(regParam)
val bcFeaturesStd = instances.context.broadcast(featuresStd)
- val costFun = new LogisticCostFun(instances, numClasses, $(fitIntercept),
- $(standardization), bcFeaturesStd, regParamL2, multinomial = isMultinomial,
+ val getAggregatorFunc = new LogisticAggregator(bcFeaturesStd, numClasses, $(fitIntercept),
+ multinomial = isMultinomial)(_)
+ val getFeaturesStd = (j: Int) => if (j >= 0 && j < numCoefficientSets * numFeatures) {
+ featuresStd(j / numCoefficientSets)
+ } else {
+ 0.0
+ }
+
+ val regularization = if (regParamL2 != 0.0) {
+ val shouldApply = (idx: Int) => idx >= 0 && idx < numFeatures * numCoefficientSets
--- End diff --
It seems that the `regularization` contains `intercept`, right?
However, the comment in [LogisticRegression.scala: 1903L](https://github.com/apache/spark/pull/18305/files#diff-3734f1689cb8a80b07974eb93de0795dL1903) is:
> // We do not apply regularization to the intercepts
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r126911464
--- Diff: mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregatorSuite.scala ---
@@ -0,0 +1,254 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.ml.optim.aggregator
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.ml.feature.Instance
+import org.apache.spark.ml.linalg.{BLAS, Matrices, Vector, Vectors}
+import org.apache.spark.ml.util.TestingUtils._
+import org.apache.spark.mllib.util.MLlibTestSparkContext
+
+class LogisticAggregatorSuite extends SparkFunSuite with MLlibTestSparkContext {
+
+ import DifferentiableLossAggregatorSuite.getClassificationSummarizers
+
+ @transient var instances: Array[Instance] = _
+ @transient var instancesConstantFeature: Array[Instance] = _
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ instances = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.5, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(4.0, 0.5))
+ )
+ instancesConstantFeature = Array(
+ Instance(0.0, 0.1, Vectors.dense(1.0, 2.0)),
+ Instance(1.0, 0.5, Vectors.dense(1.0, 1.0)),
+ Instance(2.0, 0.3, Vectors.dense(1.0, 0.5))
+ )
+ }
+
+
+ /** Get summary statistics for some data and create a new LogisticAggregator. */
+ private def getNewAggregator(
+ instances: Array[Instance],
+ coefficients: Vector,
+ fitIntercept: Boolean,
+ isMultinomial: Boolean): LogisticAggregator = {
+ val (featuresSummarizer, ySummarizer) =
+ DifferentiableLossAggregatorSuite.getClassificationSummarizers(instances)
+ val numClasses = ySummarizer.histogram.length
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val bcFeaturesStd = spark.sparkContext.broadcast(featuresStd)
+ val bcCoefficients = spark.sparkContext.broadcast(coefficients)
+ new LogisticAggregator(bcFeaturesStd, numClasses, fitIntercept, isMultinomial)(bcCoefficients)
+ }
+
+ test("aggregator add method input size") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator features dimension must match coefficients dimension") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, 1.0, Vectors.dense(2.0)))
+ }
+ }
+ }
+
+ test("negative weight") {
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ withClue("LogisticAggregator does not support negative instance weights") {
+ intercept[IllegalArgumentException] {
+ agg.add(Instance(1.0, -1.0, Vectors.dense(2.0, 1.0)))
+ }
+ }
+ }
+
+ test("check sizes multinomial") {
+ val rng = new scala.util.Random
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val coefWithIntercept = Vectors.dense(
+ Array.fill(numClasses * (numFeatures + 1))(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(
+ Array.fill(numClasses * numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(instances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = true)
+ val aggNoIntercept = getNewAggregator(instances, coefWithoutIntercept, fitIntercept = false,
+ isMultinomial = true)
+ instances.foreach(aggIntercept.add)
+ instances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === (numFeatures + 1) * numClasses)
+ assert(aggNoIntercept.gradient.size === numFeatures * numClasses)
+ }
+
+ test("check sizes binomial") {
+ val rng = new scala.util.Random
+ val binaryInstances = instances.filter(_.label < 2.0)
+ val numFeatures = binaryInstances.head.features.size
+ val coefWithIntercept = Vectors.dense(Array.fill(numFeatures + 1)(rng.nextDouble))
+ val coefWithoutIntercept = Vectors.dense(Array.fill(numFeatures)(rng.nextDouble))
+ val aggIntercept = getNewAggregator(binaryInstances, coefWithIntercept, fitIntercept = true,
+ isMultinomial = false)
+ val aggNoIntercept = getNewAggregator(binaryInstances, coefWithoutIntercept,
+ fitIntercept = false, isMultinomial = false)
+ binaryInstances.foreach(aggIntercept.add)
+ binaryInstances.foreach(aggNoIntercept.add)
+
+ assert(aggIntercept.gradient.size === numFeatures + 1)
+ assert(aggNoIntercept.gradient.size === numFeatures)
+ }
+
+ test("check correctness multinomial") {
+ /*
+ Check that the aggregator computes loss/gradient for:
+ -sum_i w_i * (beta_y dot x_i - log(sum_k e^(beta_k dot x_i)))
+ */
+ val coefArray = Array(1.0, 2.0, -2.0, 3.0, 0.0, -1.0)
+ val interceptArray = Array(4.0, 2.0, -3.0)
+ val numFeatures = instances.head.features.size
+ val numClasses = instances.map(_.label).toSet.size
+ val intercepts = Vectors.dense(interceptArray)
+ val (featuresSummarizer, ySummarizer) = getClassificationSummarizers(instances)
+ val featuresStd = featuresSummarizer.variance.toArray.map(math.sqrt)
+ val weightSum = instances.map(_.weight).sum
+
+ val agg = getNewAggregator(instances, Vectors.dense(coefArray ++ interceptArray),
+ fitIntercept = true, isMultinomial = true)
+ instances.foreach(agg.add)
+
+ // compute the loss
+ val stdCoef = coefArray.indices.map(i => coefArray(i) / featuresStd(i / numClasses)).toArray
--- End diff --
That sounds weird actually. It means there is no point in having the `standardization` param? If it's `false` then actually the loss gradients are computed in the scaled space regardless, while the reg gradient is not?
That doesn't sound like expected behavior if a user explicitly sets `standardization=false`.
```scala
scala> val lr = new LogisticRegression()
scala> val model1 = lr.setStandardization(false).setRegParam(0.0).fit(df)
scala> val model2 = lr.setStandardization(true).setRegParam(0.0).fit(df)
scala> val model3 = lr.setStandardization(false).setRegParam(0.1).fit(df)
scala> val model4 = lr.setStandardization(true).setRegParam(0.1).fit(df)
scala> model1.coefficients == model2.coefficients
res0: Boolean = true
scala> model3.coefficients == model4.coefficients
res1: Boolean = false
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r127972263
--- Diff: mllib/src/main/scala/org/apache/spark/ml/classification/LogisticRegression.scala ---
@@ -598,8 +598,23 @@ class LogisticRegression @Since("1.2.0") (
val regParamL2 = (1.0 - $(elasticNetParam)) * $(regParam)
val bcFeaturesStd = instances.context.broadcast(featuresStd)
- val costFun = new LogisticCostFun(instances, numClasses, $(fitIntercept),
- $(standardization), bcFeaturesStd, regParamL2, multinomial = isMultinomial,
+ val getAggregatorFunc = new LogisticAggregator(bcFeaturesStd, numClasses, $(fitIntercept),
+ multinomial = isMultinomial)(_)
+ val getFeaturesStd = (j: Int) => if (j >= 0 && j < numCoefficientSets * numFeatures) {
+ featuresStd(j / numCoefficientSets)
+ } else {
+ 0.0
+ }
+
+ val regularization = if (regParamL2 != 0.0) {
+ val shouldApply = (idx: Int) => idx >= 0 && idx < numFeatures * numCoefficientSets
--- End diff --
Thanks for clarifying.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r127934107
--- Diff: mllib/src/main/scala/org/apache/spark/ml/classification/LogisticRegression.scala ---
@@ -598,8 +598,23 @@ class LogisticRegression @Since("1.2.0") (
val regParamL2 = (1.0 - $(elasticNetParam)) * $(regParam)
val bcFeaturesStd = instances.context.broadcast(featuresStd)
- val costFun = new LogisticCostFun(instances, numClasses, $(fitIntercept),
- $(standardization), bcFeaturesStd, regParamL2, multinomial = isMultinomial,
+ val getAggregatorFunc = new LogisticAggregator(bcFeaturesStd, numClasses, $(fitIntercept),
+ multinomial = isMultinomial)(_)
+ val getFeaturesStd = (j: Int) => if (j >= 0 && j < numCoefficientSets * numFeatures) {
+ featuresStd(j / numCoefficientSets)
+ } else {
+ 0.0
+ }
+
+ val regularization = if (regParamL2 != 0.0) {
+ val shouldApply = (idx: Int) => idx >= 0 && idx < numFeatures * numCoefficientSets
--- End diff --
The intercepts are appended to the coefficient vectors, so the `idx` for intercept will be `>= numFeatures * numCoefficientSets`. Hence this function ignores intercept reg.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18305: [SPARK-20988][ML] Logistic regression uses aggreg...
Posted by MLnick <gi...@git.apache.org>.
Github user MLnick commented on a diff in the pull request:
https://github.com/apache/spark/pull/18305#discussion_r124016865
--- Diff: mllib/src/main/scala/org/apache/spark/ml/regression/LinearRegression.scala ---
@@ -322,10 +322,11 @@ class LinearRegression @Since("1.3.0") (@Since("1.3.0") override val uid: String
val getAggregatorFunc = new LeastSquaresAggregator(yStd, yMean, $(fitIntercept),
bcFeaturesStd, bcFeaturesMean)(_)
+ val getFeaturesStd = (j: Int) => if (j >=0 && j < numFeatures) featuresStd(j) else 0.0
--- End diff --
space between `>=0`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org