[SPARK-20906][SparkR]:Constrained Logistic Regression for SparkR #18128

wangmiao1981 · 2017-05-27T07:31:22Z

What changes were proposed in this pull request?

PR #17715 Added Constrained Logistic Regression for ML. We should add it to SparkR.

How was this patch tested?

Add new unit tests.

SparkQA · 2017-05-27T07:36:43Z

Test build #77455 has finished for PR 18128 at commit 7627ac9.

This patch fails R style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-05-27T07:46:42Z

Test build #77456 has finished for PR 18128 at commit 9d302c4.

This patch fails R style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-05-27T08:48:14Z

Test build #77457 has finished for PR 18128 at commit 6a5b568.

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

SparkQA · 2017-05-27T17:23:03Z

Test build #77462 has finished for PR 18128 at commit 354eeb3.

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

felixcheung · 2017-05-27T19:34:02Z

@yanboliang

felixcheung · 2017-05-27T19:38:06Z

R/pkg/R/mllib_classification.R

could you add some check for upperBoundsOnCoefficients or lowerBoundsOnCoefficients
for example, if upperBoundsOnCoefficients is a vector instead of a matrix, ncol(as.array(upperBoundsOnCoefficients)) will be NA

SparkQA · 2017-05-29T07:03:37Z

Test build #77495 has finished for PR 18128 at commit 8094b8d.

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

wangmiao1981 · 2017-05-31T20:39:23Z

@yanboliang Can you take a look? Thanks!

SparkQA · 2017-05-31T22:08:08Z

Test build #77607 has finished for PR 18128 at commit 4c7b97c.

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

yanboliang · 2017-06-01T06:37:54Z

@wangmiao1981 Will review soon. Thanks.

yanboliang · 2017-06-02T06:12:02Z

R/pkg/R/mllib_classification.R

Here we can merge the four parameters into two: nrow and ncol, since the matrix format of lowerBoundsOnCoefficients and upperBoundsOnCoefficients should be consistent. If the input doesn't confirm with this rule, we need to throw exception.

Question: Based on my understanding, lowerBoundsOnCoefficients and upperBoundsOnCoefficients are not required to set at the same time. They can be set at the same time.
For the first case, we can't enforce the dimension of the two matrices because one could be NULL.
For the second case, we can check it.

So, we can't enforce the rule in general.

Oh, I think I can do the check because I have a NULL check before enforcing the rule.

yanboliang · 2017-06-02T06:18:46Z

mllib/src/main/scala/org/apache/spark/ml/r/LogisticRegressionWrapper.scala

See my comment above, we can merge the four parameters into two: numRowsOfBoundsOnCoefficients and numColsOfBoundsOnCoefficients, please follow Scala naming convention.

yanboliang · 2017-06-02T06:20:46Z

R/pkg/inst/tests/testthat/test_mllib_classification.R

Could you add test for multinomial logistic regression? I think only test one side bound is enough.

SparkQA · 2017-06-02T23:22:26Z

Test build #77691 has finished for PR 18128 at commit c304ba8.

This patch fails R style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-06-03T00:46:40Z

Test build #77695 has finished for PR 18128 at commit 47fbccc.

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

felixcheung · 2017-06-03T04:04:24Z

R/pkg/R/mllib_classification.R

paste would insert space - use paste0 instead or just remove the space in the string to let it to add it

felixcheung · 2017-06-03T04:06:29Z

R/pkg/R/mllib_classification.R

nit: no need to as.integer(row) and as.integer(col) since they are set internally and not a parameter

felixcheung · 2017-06-03T04:08:55Z

R/pkg/R/mllib_classification.R

given how this is used later in scala code, should there be a check that nrow(upper) == nrow(lower) and ditto for ncol(upper) == ncol(lower)?

This is the case where we only set the upperbound. We can set both or either one of them.

For the case that both are set. We enforce upperbound and lowerbound are the same dimension, as checked above.

ok thanks, L290-291

SparkQA · 2017-06-03T05:30:40Z

Test build #77699 has finished for PR 18128 at commit b89a0f7.

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

wangmiao1981 · 2017-06-03T17:34:11Z

Jenkins retest this please

SparkQA · 2017-06-03T18:35:42Z

Test build #77703 has finished for PR 18128 at commit b89a0f7.

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

wangmiao1981 · 2017-06-03T20:59:18Z

Local test passed. Let me check it tonight.

wangmiao1981 · 2017-06-03T23:12:33Z

@felixcheung if I remove as.integer, backend doesn't recognize it as integer.

SparkQA · 2017-06-04T00:13:26Z

Test build #77706 has finished for PR 18128 at commit 2a7e6e3.

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

felixcheung · 2017-06-04T17:49:54Z

ok, thanks, i guess it could be set as 0L but this is good.

SparkQA · 2017-06-12T04:44:12Z

Test build #77909 has finished for PR 18128 at commit c3190b5.

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

wangmiao1981 · 2017-06-13T10:36:27Z

ping @yanboliang

yanboliang · 2017-06-13T17:13:30Z

@wangmiao1981 I'm at DataWorks Summit today and tomorrow, will take a look by this Friday. Thanks.

felixcheung

minor comment, LGTM otherwise

felixcheung · 2017-06-15T07:21:52Z

R/pkg/R/mllib_classification.R

size must be equal with 1 - should this be size must be equal to 1?

felixcheung · 2017-06-15T07:22:00Z

R/pkg/R/mllib_classification.R

ditto here size must be equal with 1

felixcheung · 2017-06-15T07:25:52Z

R/pkg/R/mllib_classification.R

while logically and semantically correct here, might prefer && and || instead of & and | here for programmability/correctness in case something becomes length > 1 here later

SparkQA · 2017-06-16T10:10:00Z

Test build #78172 has finished for PR 18128 at commit 45b62cc.

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

felixcheung

LGTM again.
@yanboliang do you want to take a last look before merging this?

felixcheung · 2017-06-22T03:43:00Z

merged to master.

## What changes were proposed in this pull request? PR apache#17715 Added Constrained Logistic Regression for ML. We should add it to SparkR. ## How was this patch tested? Add new unit tests. Author: wangmiao1981 <[email protected]> Closes apache#18128 from wangmiao1981/test.

felixcheung reviewed May 27, 2017

View reviewed changes

wangmiao1981 force-pushed the test branch from 8094b8d to 4c7b97c Compare May 31, 2017 21:04

yanboliang reviewed Jun 2, 2017

View reviewed changes

felixcheung reviewed Jun 3, 2017

View reviewed changes

wangmiao1981 force-pushed the test branch from 2a7e6e3 to c3190b5 Compare June 12, 2017 03:35

felixcheung approved these changes Jun 15, 2017

View reviewed changes

wangmiao1981 added 3 commits June 16, 2017 16:43

add constraint logit

602c27f

add unit test and doc

2dd0b7d

fix <-

5b272fd

wangmiao1981 added 9 commits June 16, 2017 16:43

fix style

eb65a1d

change test order

62a436b

add check and test for matrix type

43640f4

extra spaces

9ab89c7

address review comments

b8d683c

add unit test and fix R style issue

98709ac

address review comments

8c88a4f

revert change of as.integer

8bebb98

address review comments

45b62cc

wangmiao1981 force-pushed the test branch from c3190b5 to 45b62cc Compare June 16, 2017 09:06

felixcheung approved these changes Jun 19, 2017

View reviewed changes

asfgit closed this in 5354337 Jun 22, 2017

[SPARK-20906][SparkR]:Constrained Logistic Regression for SparkR #18128

[SPARK-20906][SparkR]:Constrained Logistic Regression for SparkR #18128

Uh oh!

Conversation

wangmiao1981 commented May 27, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented May 27, 2017

Uh oh!

SparkQA commented May 27, 2017

Uh oh!

SparkQA commented May 27, 2017

Uh oh!

SparkQA commented May 27, 2017

Uh oh!

felixcheung commented May 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 29, 2017

Uh oh!

wangmiao1981 commented May 31, 2017

Uh oh!

SparkQA commented May 31, 2017

Uh oh!

yanboliang commented Jun 1, 2017

Uh oh!

yanboliang Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 2, 2017

Uh oh!

SparkQA commented Jun 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 3, 2017

Uh oh!

wangmiao1981 commented Jun 3, 2017

Uh oh!

SparkQA commented Jun 3, 2017

Uh oh!

wangmiao1981 commented Jun 3, 2017

Uh oh!

wangmiao1981 commented Jun 3, 2017

Uh oh!

SparkQA commented Jun 4, 2017

Uh oh!

felixcheung commented Jun 4, 2017

Uh oh!

SparkQA commented Jun 12, 2017

Uh oh!

wangmiao1981 commented Jun 13, 2017

Uh oh!

yanboliang commented Jun 13, 2017

Uh oh!

felixcheung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

yanboliang Jun 2, 2017 •

edited

Loading