[Spark-24024][ML] Fix poisson deviance calculations in GLM to handle y = 0 #21125
Conversation
|
ok to test |
dbtsai
left a comment
dbtsai
left a comment
There was a problem hiding this comment.
Only couple small comments, and we're ready to merge it once they're resolved.
Thanks.
DB Tsai | Siri Open Source Technologies | Apple, Inc
| private def ylogy(y: Double, mu: Double): Double = { | ||
| if (y == 0) 0.0 else y * math.log(y / mu) | ||
| } | ||
|
|
There was a problem hiding this comment.
Another ylogy implementation in Binomial. Can you move this code to object GeneralizedLinearRegression and make it private to this package?
There was a problem hiding this comment.
Thanks so much for the quick review. I have moved the ylog implementation to object GeneralizedLinearRegression. One quick question here: I am not sure I have fully understood why this is the right place for ylog? Thanks!
There was a problem hiding this comment.
Any suggestion to avoid the duplicated code? Let's followup this later if you have an idea.
| print(as.vector(coef(model))) | ||
| } | ||
| [1] -0.0457441 -0.6833928 | ||
| [1] 1.8121235 -0.1747493 -0.5815417 |
There was a problem hiding this comment.
Can you update the R script which generate the deviance?
There was a problem hiding this comment.
Updated. The updated script is sufficient to calculate deviance on its own.
| Vectors.dense(1.8121235, -0.1747493, -0.5815417)) | ||
| Vectors.dense(0.0, -0.0457441, -0.6833928, 3.8093), | ||
| Vectors.dense(1.8121235, -0.1747493, -0.5815417, 3.7006)) | ||
|
|
There was a problem hiding this comment.
Adding them to expected is not consistent to the rest of the test code.
How about
val residualDeviancesR = Array(3.8093, 3.7006)
| val actual = Vectors.dense(model.intercept, model.coefficients(0), model.coefficients(1), | ||
| model.summary.deviance) | ||
| assert(actual ~= expected(idx) absTol 1e-4, "Model mismatch: GLM with poisson family, " + | ||
| s"$link link and fitIntercept = $fitIntercept (with zero values).") |
There was a problem hiding this comment.
assert(model.summary.deviance ~== residualDeviancesR(idx) absTol 1E-3)
|
Test build #89699 has finished for PR 21125 at commit
|
tengpeng
changed the title
|
Test build #89723 has finished for PR 21125 at commit
|
|
LGTM, merged into master. Thanks. DB Tsai | Siri Open Source Technologies | Apple, Inc |
4 participants
What changes were proposed in this pull request?
It is reported by Spark users that the deviance calculation for poisson regression does not handle y = 0. Thus, the correct model summary cannot be obtained. The user has confirmed the the issue is in
The user also mentioned there are many other places he believe we should check the same thing. However, no other changes are needed, including Gamma distribution.
How was this patch tested?
Add a comparison with R deviance calculation to the existing unit test.