Uh oh!
There was an error while loading. Please reload this page.
[SPARK-19133][SPARKR][ML] fix glm for Gamma, clarify glm family supported - #16511
[SPARK-19133][SPARKR][ML] fix glm for Gamma, clarify glm family supported#16511felixcheung wants to merge 3 commits into
Conversation
| #' the result of a call to a family function. Refer R family at | ||
| #' \url{https://stat.ethz.ch/R-manual/R-devel/library/stats/html/family.html}. | ||
| #' Currently these families are supported: \code{binomial}, \code{gaussian}, | ||
| #' \code{Gamma}, and \code{poisson}. |
There was a problem hiding this comment.
in R, Gamma family is capital G
felixcheung
commented
Jan 9, 2017
SparkQA
commented
Jan 9, 2017
Test build #71060 has finished for PR 16511 at commit
|
felixcheung
commented
Jan 9, 2017
turns out Gamma is broken. will be working on adding tests |
SparkQA
commented
Jan 9, 2017
Test build #71066 has finished for PR 16511 at commit
|
SparkQA
commented
Jan 9, 2017
Test build #71070 has finished for PR 16511 at commit
|
| # For known families, Gamma is upper-cased | ||
| jobj <- callJStatic("org.apache.spark.ml.r.GeneralizedLinearRegressionWrapper", | ||
| "fit", formula, data@sdf, family$family, family$link, | ||
| "fit", formula, data@sdf, tolower(family$family), family$link, |
There was a problem hiding this comment.
I think we should put case conversion into MLlib side, then we can get consistent behavior from APIs across different languages (Scala/Python/R). I wish you would not mind that I have sent #16516 to fix it in MLlib. If it's reasonable, we can revert the change of this line.
There was a problem hiding this comment.
that's a good idea - given how this is likely broken since Spark 1.6 I'd like to get the fix in ASAP and to master, branch-2.0, branch-2.1, or even perhaps branch-1.6 just in case.
I'd probably need to open different PRs for these since our file organization has changed, and would need to keep the R side change - unless you think ML side change could go to these branches too?
There was a problem hiding this comment.
I think we need take some time to get MLlib side changed (and it involves changes for other estimators). Sounds good to keep the R side change.
| x <- runif(100, -1, 1) | ||
| y <- rgamma(100, rate = 10 / exp(0.5 + 1.2 * x), shape = 10) | ||
| df <- as.DataFrame(as.data.frame(list(x = x, y = y))) | ||
| model <- glm(y ~ x, family = Gamma, df) |
There was a problem hiding this comment.
I'd prefer to use spark.glm here, since the title of this test is: spark.glm and predict(see L52). We have separate tests for R-compliant method glm, but it's not necessary to duplicate all tests.
There was a problem hiding this comment.
I'm not sure it matters much - as you can see in the code glm is a single-line wrapper for spark.glm - I actually thought it was better to add some tests for glm instead of just testing spark.glm
yanboliang
commented
Jan 10, 2017
LGTM |
…rted ## What changes were proposed in this pull request? R family is a longer list than what Spark supports. ## How was this patch tested? manual Author: Felix Cheung <felixcheung_m@hotmail.com> Closesapache#16511 from felixcheung/rdocglmfamily.
…rted ## What changes were proposed in this pull request? R family is a longer list than what Spark supports. ## How was this patch tested? manual Author: Felix Cheung <felixcheung_m@hotmail.com> Closesapache#16511 from felixcheung/rdocglmfamily.
What changes were proposed in this pull request?
R family is a longer list than what Spark supports.
How was this patch tested?
manual