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?

## Advertising

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