[SPARK-5099][Mllib] Simplify logistic loss function

[viirya]

This is a minor pr where I think that we can simply take minus of margin, instead of subtracting margin.

Mathematically, they are equal. But the modified equation is the common form of logistic loss function and so more readable. It also computes more accurate value as some quick tests show.

[srowen]

+1 looks like a small good improvement.

[SparkQA]

Test build #25053 has finished for PR 3899 at commit 2bc5712.

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

[mengxr]

@viirya Thanks for the improvement! We could make this computation more accurate. We should branch based on sign(label * margin), similar to

https://github.com/apache/spark/blob/master/mllib/src/main/scala/org/apache/spark/mllib/tree/loss/LogLoss.scala#L64

Do you have time making this change? Or I can merge this PR and create a JIRA as a reminder.

[viirya]

@mengxr Thanks for that. I will do it later.

[viirya]

Hi @mengxr, Thanks for comment. I may be wrong, but I think that we should branch based on sign(label) instead of sign(label * margin)?

Because according to the definition of logistic loss function, log(1 + exp(-yp)), where y = +1 for positive label and y = -1 for negative label.

When we branch on sign(label * margin), we can not make sure what the sign of label is, right? Because when sign(label * margin) == 1, it could be (sign(label), sign(margin)) == (1, 1) or (sign(label), sign(margin)) == (-1, -1).

Besides, https://github.com/apache/spark/blob/master/mllib/src/main/scala/org/apache/spark/mllib/tree/loss/LogLoss.scala#L67 and https://github.com/apache/spark/blob/master/mllib/src/main/scala/org/apache/spark/mllib/tree/loss/LogLoss.scala#L69 are mathematically the same equation. I think it should be a bug. As the function note said, it is not used by the gradient boosting algorithm but only for debugging, so it is not found before. I add a commit to fix it.

[SparkQA]

Test build #25093 has finished for PR 3899 at commit a3f83ca.

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

[mengxr]

@viirya They are the same analytically but not numerically, for example,

scala> math.log1p(math.exp(1000))
res2: Double = Infinity

scala> 1000 + math.log1p(math.exp(-1000))
res3: Double = 1000.0

[viirya]

OK. I understood it. I am wrong for LogLoss. Will revert it back.

[viirya]

@mengxr I noticed that you file a issue in #SPARK-5101. Do I need to extract the codes in this pr to the place you suggested mllib.util.MathFunctions?

[SparkQA]

Test build #25097 has finished for PR 3899 at commit 72a295e.

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

[viirya]

@mengxr I think I already know what you meant to branch based on sign(label * margin). I made some modifications.

[SparkQA]

Test build #25101 has finished for PR 3899 at commit 0aa51e4.

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

[jkbradley]

This looks like you expect labels to be -1, +1. They are actually 0, 1.

[viirya]

Oh? I thought it is similar to LogLoss so it would be {+1, -1}. Fixed.

[mengxr]

LGTM pending Jenkins. I'm going to merge this first. We might want to add more functions in #3915 .

[SparkQA]

Test build #25127 has finished for PR 3899 at commit 91a3860.

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

[viirya]

Thanks.

[mengxr]

Merged into master. Thanks!