From reviews-return-622271-archive-asf-public=cust-asf.ponee.io@spark.apache.org Tue Mar 6 11:40:36 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 13E8B180652 for ; Tue, 6 Mar 2018 11:40:35 +0100 (CET) Received: (qmail 55766 invoked by uid 500); 6 Mar 2018 10:40:35 -0000 Mailing-List: contact reviews-help@spark.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Delivered-To: mailing list reviews@spark.apache.org Received: (qmail 55749 invoked by uid 99); 6 Mar 2018 10:40:34 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 06 Mar 2018 10:40:34 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 66341F2178; Tue, 6 Mar 2018 10:40:34 +0000 (UTC) From: jrdi To: reviews@spark.apache.org Reply-To: reviews@spark.apache.org References: In-Reply-To: Subject: [GitHub] spark pull request #7080: [SPARK-8700][ML] Disable feature scaling in Logist... Content-Type: text/plain Message-Id: <20180306104034.66341F2178@git1-us-west.apache.org> Date: Tue, 6 Mar 2018 10:40:34 +0000 (UTC) Github user jrdi commented on a diff in the pull request: https://github.com/apache/spark/pull/7080#discussion_r171688655 --- Diff: mllib/src/main/scala/org/apache/spark/ml/classification/LogisticRegression.scala --- @@ -534,27 +559,43 @@ private class LogisticCostFun( case (aggregator1, aggregator2) => aggregator1.merge(aggregator2) }) - // regVal is the sum of weight squares for L2 regularization - val norm = if (regParamL2 == 0.0) { - 0.0 - } else if (fitIntercept) { - brzNorm(Vectors.dense(weights.toArray.slice(0, weights.size -1)).toBreeze, 2.0) - } else { - brzNorm(weights, 2.0) - } - val regVal = 0.5 * regParamL2 * norm * norm + val totalGradientArray = logisticAggregator.gradient.toArray - val loss = logisticAggregator.loss + regVal - val gradient = logisticAggregator.gradient - - if (fitIntercept) { - val wArray = w.toArray.clone() - wArray(wArray.length - 1) = 0.0 - axpy(regParamL2, Vectors.dense(wArray), gradient) + // regVal is the sum of weight squares excluding intercept for L2 regularization. + val regVal = if (regParamL2 == 0.0) { + 0.0 } else { - axpy(regParamL2, w, gradient) + var sum = 0.0 + w.foreachActive { (index, value) => + // If `fitIntercept` is true, the last term which is intercept doesn't + // contribute to the regularization. + if (index != numFeatures) { + // The following code will compute the loss of the regularization; also + // the gradient of the regularization, and add back to totalGradientArray. + sum += { + if (standardization) { + totalGradientArray(index) += regParamL2 * value + value * value + } else { + if (featuresStd(index) != 0.0) { + // If `standardization` is false, we still standardize the data + // to improve the rate of convergence; as a result, we have to + // perform this reverse standardization by penalizing each component + // differently to get effectively the same objective function when + // the training dataset is not standardized. + val temp = value / (featuresStd(index) * featuresStd(index)) + totalGradientArray(index) += regParamL2 * temp + value * temp --- End diff -- @dbtsai I understand this change was implemented years ago but I don't get why are you creating `temp` as `value / std^2`. Is not supposed to be standardized? Shouldn't be `value / std`? In addition, `value * temp` is equivalent to `(value / std)^2` which makes more sense to me. Comparing it to the standardized method where we are adding `regParamL2 * standardizeValue` to the gradient and 'returning' `standardizeValue^2`, here we are adding `regParamL2 * standardizeValue / std` and returning `standardizeValue^2`. Does it makes sense? Am I missing some point here? --- --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org For additional commands, e-mail: reviews-help@spark.apache.org