Skip to content

[SPARK-21622][ML][SparkR] Support offset in SparkR GLM - #18831

Closed
actuaryzhang wants to merge 3 commits into
apache:masterfrom
actuaryzhang:sparkROffset
Closed

[SPARK-21622][ML][SparkR] Support offset in SparkR GLM#18831
actuaryzhang wants to merge 3 commits into
apache:masterfrom
actuaryzhang:sparkROffset

Conversation

@actuaryzhang

@actuaryzhangactuaryzhang commented Aug 3, 2017

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Support offset in SparkR GLM #16699

@SparkQA

SparkQA commented Aug 3, 2017

Copy link
Copy Markdown

Test build #80194 has finished for PR 18831 at commit 6ec068e.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@actuaryzhang

actuaryzhang commented Aug 3, 2017

Copy link
Copy Markdown
ContributorAuthor

Jenkins, retest this please

@SparkQA

SparkQA commented Aug 3, 2017

Copy link
Copy Markdown

Test build #80213 has finished for PR 18831 at commit 6ec068e.

  • This patch fails SparkR unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

felixcheung
felixcheung reviewed Aug 3, 2017
View reviewed changes
Comment threadR/pkg/R/mllib_regression.R Outdated
setMethod("spark.glm", signature(data = "SparkDataFrame", formula = "formula"),
function(data, formula, family = gaussian, tol = 1e-6, maxIter = 25, weightCol = NULL,
regParam = 0.0, var.power = 0.0, link.power = 1.0 - var.power,
offsetCol = NULL, regParam = 0.0, var.power = 0.0, link.power = 1.0 - var.power,

@felixcheungfelixcheungAug 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd avoid adding a param in the middle - it breaks code passing param by order

Comment threadR/pkg/R/mllib_regression.R
offsetCol <- NULL
} else if (!is.null(offsetCol)) {
offsetCol <- as.character(offsetCol)
}

@felixcheungfelixcheungAug 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps

if (!is.null(offsetCol)) {
offsetCol <- as.character(offsetCol)
if (nchar(offsetCol) == 0) {
offsetCol <- NULL
}
}

not sure if you want to cover other cases when offsetCol cannot be coerced - eg. NA

@SparkQA

SparkQA commented Aug 3, 2017

Copy link
Copy Markdown

Test build #80218 has finished for PR 18831 at commit dc8ccbc.

  • This patch fails SparkR unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Aug 3, 2017

Copy link
Copy Markdown

Test build #80219 has finished for PR 18831 at commit 3c4ebf9.

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

@actuaryzhang

actuaryzhang commented Aug 4, 2017

Copy link
Copy Markdown
ContributorAuthor

Thanks for your comments, Felix.
Addressed all issues.
@yanboliang Could you take a quick look?

felixcheung
felixcheung reviewed Aug 4, 2017
View reviewed changes
Comment threadR/pkg/tests/fulltests/test_mllib_regression.R
stats <- summary(spark.glm(training, Sepal_Width ~ Sepal_Length + Species,
family = poisson(), offsetCol = "Petal_Length"))
rStats <- suppressWarnings(summary(glm(Sepal.Width ~ Sepal.Length + Species,
data = iris, family = poisson(), offset = iris$Petal.Length)))

@felixcheungfelixcheungAug 4, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's interesting - perhaps we should take col in addition to col name too

@actuaryzhangactuaryzhangAug 4, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then do you want to make the change for weight as well?

@felixcheungfelixcheungAug 5, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably across every in ml.
let's discuss this in a new JIRA.

@yanboliangyanboliangAug 5, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote to keep the name as it is, because it's the column name of offset rather than the offset itself. weightCol is the same. We would like to keep SparkR MLlib wrappers' argument name consistent with R only when it's applicable. I'm ok to create a new JIRA to discuss it. Thanks.

yanboliang
yanboliang approved these changes Aug 5, 2017
View reviewed changes

@yanboliangyanboliang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@felixcheung

felixcheung commented Aug 5, 2017 via email

Copy link
Copy Markdown
Member

@yanboliang

yanboliang commented Aug 5, 2017

Copy link
Copy Markdown
Contributor

@felixcheung Sorry for misunderstand, I agree we can support df$myoffset as well, the requirement make sense for R users. Let's create a separate JIRA to track it and do this change for other similar arguments like weightCol as well. Thanks.

@actuaryzhang

actuaryzhang commented Aug 6, 2017•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Thanks both of you for the comments. Yes, I think it's best to keep this PR on offset and we can address the other improvements later.

@felixcheung

felixcheung commented Aug 6, 2017

Copy link
Copy Markdown
Member

merged to master

@asfgitasfgit closed this in 55aa4daAug 6, 2017
@actuaryzhang
actuaryzhang deleted the sparkROffset branch August 7, 2017 00:09
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixcheungfelixcheungfelixcheung approved these changes
+1 more reviewer
@yanboliangyanboliangyanboliang approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@actuaryzhang@SparkQA@felixcheung@yanboliang