[SPARK-21306][ML] OneVsRest should support setWeightCol #18554

facaiy · 2017-07-06T09:58:17Z

What changes were proposed in this pull request?

add setWeightCol method for OneVsRest.

weightCol is ignored if classifier doesn't inherit HasWeightCol trait.

How was this patch tested?

add an unit test.

lins05 · 2017-07-06T16:39:05Z

I guess we also need to update the python part: https://github.com/apache/spark/blob/v2.2.0-rc6/python/pyspark/ml/classification.py#L1563

facaiy · 2017-07-06T22:03:16Z

@lins05 thanks, reasonable suggestion, I will fix it later.

facaiy · 2017-07-07T04:26:38Z

I'm not familiar with R, and use git grep to search "OneVsRest" and get nothing. Hence it seems that nothing is needed to do with R part.

MLnick · 2017-07-07T07:32:59Z

ok to test

SparkQA · 2017-07-07T08:08:27Z

Test build #79325 has finished for PR 18554 at commit 25d681f.

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

SparkQA · 2017-07-07T08:49:22Z

Test build #79330 has finished for PR 18554 at commit e511b90.

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

SparkQA · 2017-07-07T10:07:31Z

Test build #79331 has finished for PR 18554 at commit 1c215f3.

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

facaiy · 2017-07-11T06:18:06Z

@srowen @yanboliang Could you help review the PR? Thanks.

yanboliang · 2017-07-11T14:46:57Z

mllib/src/main/scala/org/apache/spark/ml/classification/OneVsRest.scala

+      case c: HasWeightCol if c.isDefined(c.weightCol) && c.getWeightCol.nonEmpty =>
+        dataset.select($(labelCol), $(featuresCol), c.getWeightCol)
+      case _ => dataset.select($(labelCol), $(featuresCol))
+    }


OneVsRest is a classification estimator, I think we should make weightCol a member param of it like featuresCol. For example:

val dataset = dataset // This dataset has column: a, b, c. val ova = new OneVsRest().setFeaturesCol("a").setClassifier(new LogisticRegression().setFeaturesCol("b"))

The features column used by OneVsRest is a. The features column set for OneVsRest will override corresponding set in OneVsRest.classifier. So we should follow this way for weightCol as well. Thanks.

Hi, @yanboliang . As @MLnick said, not all classifiers inherits HasWeightCol, so it might cause confusion.

In my opinion, setWeightCol is an attribute owned by one specific classifier itself, like setProbabilityCol and setRawPredictionCol for Logistic Regreesion. So I'd suggest that user should configure the classifier itself, rather than OneVsRest.

@facaiy It doesn't matter. If the classifier doesn't inherit from HasWeightCol, we don't run setWeightCol for that classifier but to print out warning log to say weightCol doesn't take effect. You can refer these lines of code to learn how featuresCol be handled. We can do it in similar way. Thanks.

MLnick · 2017-07-11T14:58:48Z

I agree on this

…

On Tue, 11 Jul 2017 at 16:53, Yanbo Liang ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In mllib/src/main/scala/org/apache/spark/ml/classification/OneVsRest.scala <#18554 (comment)>: > @@ -317,7 +318,12 @@ final class OneVsRest @SInCE("1.4.0") ( val numClasses = MetadataUtils.getNumClasses(labelSchema).fold(computeNumClasses())(identity) instr.logNumClasses(numClasses) - val multiclassLabeled = dataset.select($(labelCol), $(featuresCol)) + val multiclassLabeled = getClassifier match { + // SPARK-21306: cache weightCol if necessary + case c: HasWeightCol if c.isDefined(c.weightCol) && c.getWeightCol.nonEmpty => + dataset.select($(labelCol), $(featuresCol), c.getWeightCol) + case _ => dataset.select($(labelCol), $(featuresCol)) + } OneVsRest is a classification estimator, I think we should make weightCol a member param of it like featuresCol. For example: val dataset = dataset // This dataset has column: a, b, c. val ova = new OneVsRest().setFeaturesCol("a").setClassifier(new LogisticRegression().setFeaturesCol("b")) The features column used by OneVsRest is a. The features column set for OneVsRest will override corresponding set in OneVsRest.classifier. So we should follow this way for weightCol as well. Thanks. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#18554 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_SBzWk_xBWbVhuw3Q60BJdQnvPlKnAks5sM4xsgaJpZM4OPag0> .

MLnick · 2017-07-11T15:00:28Z

Oh, of course some classifiers may not have weight col. in which case it will be ignored - though we should probably log a warning in this case. On Tue, 11 Jul 2017 at 16:58, Nick Pentreath <[email protected]> wrote:

…

I agree on this On Tue, 11 Jul 2017 at 16:53, Yanbo Liang ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In > mllib/src/main/scala/org/apache/spark/ml/classification/OneVsRest.scala > <#18554 (comment)>: > > > @@ -317,7 +318,12 @@ final class OneVsRest @SInCE("1.4.0") ( > val numClasses = MetadataUtils.getNumClasses(labelSchema).fold(computeNumClasses())(identity) > instr.logNumClasses(numClasses) > > - val multiclassLabeled = dataset.select($(labelCol), $(featuresCol)) > + val multiclassLabeled = getClassifier match { > + // SPARK-21306: cache weightCol if necessary > + case c: HasWeightCol if c.isDefined(c.weightCol) && c.getWeightCol.nonEmpty => > + dataset.select($(labelCol), $(featuresCol), c.getWeightCol) > + case _ => dataset.select($(labelCol), $(featuresCol)) > + } > > OneVsRest is a classification estimator, I think we should make weightCol > a member param of it like featuresCol. For example: > > val dataset = dataset // This dataset has column: a, b, c. > val ova = new OneVsRest().setFeaturesCol("a").setClassifier(new LogisticRegression().setFeaturesCol("b")) > > The features column used by OneVsRest is a. The features column set for > OneVsRest will override corresponding set in OneVsRest.classifier. So we > should follow this way for weightCol as well. Thanks. > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#18554 (review)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AA_SBzWk_xBWbVhuw3Q60BJdQnvPlKnAks5sM4xsgaJpZM4OPag0> > . >

SparkQA · 2017-07-13T08:54:33Z

Test build #79579 has finished for PR 18554 at commit a57f096.

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

SparkQA · 2017-07-13T09:22:08Z

Test build #79581 has finished for PR 18554 at commit 54e0fca.

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

holdenk · 2017-07-19T00:02:16Z

python/pyspark/ml/tests.py

        output = model.transform(df)
        self.assertEqual(output.columns, ["label", "features", "prediction"])

+    def test_support_for_weightCol(self):


Would it make sense to also test with a classifier that doesn't have a weight col?

Sure. Use DecisionTreeClassifier to test.

SparkQA · 2017-07-19T06:57:44Z

Test build #79740 has finished for PR 18554 at commit 9ba0e2b.

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

facaiy · 2017-07-26T06:26:00Z

ping @holdenk @yanboliang

yanboliang

Left some minor comments, otherwise, LGTM.
cc @holdenk for double check. Thanks.

yanboliang · 2017-07-26T10:12:13Z

python/pyspark/ml/classification.py

    @keyword_only
    def __init__(self, featuresCol="features", labelCol="label", predictionCol="prediction",
-                 classifier=None):
+                 weightCol=None, classifier=None):


Usually we append new argument at the end, as users may use non-keyword argument, we should not break users' existing code. FYI: https://docs.python.org/3/tutorial/controlflow.html#keyword-arguments

yanboliang · 2017-07-26T10:16:54Z

python/pyspark/ml/tests.py

+        lr = LogisticRegression(maxIter=5, regParam=0.01)
+        ovr = OneVsRest(classifier=lr, weightCol="weight")
+        self.assertIsNotNone(ovr.fit(df))
+        ovr2 = OneVsRest(classifier=lr).setWeightCol("weight")


Minor: We can remove test of ovr2 and ovr4, setting param in different way will run the same code at backend.

SparkQA · 2017-07-26T13:51:00Z

Test build #79964 has finished for PR 18554 at commit 8c0beba.

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

yanboliang · 2017-07-28T02:12:39Z

Since this is a critical bug, I'll merge into master/branch-2.2/branch-2.1/branch-2.0, thanks for all.

## What changes were proposed in this pull request? add `setWeightCol` method for OneVsRest. `weightCol` is ignored if classifier doesn't inherit HasWeightCol trait. ## How was this patch tested? + [x] add an unit test. Author: Yan Facai (颜发才) <[email protected]> Closes #18554 from facaiy/BUG/oneVsRest_missing_weightCol. (cherry picked from commit a5a3189) Signed-off-by: Yanbo Liang <[email protected]>

## What changes were proposed in this pull request? add `setWeightCol` method for OneVsRest. `weightCol` is ignored if classifier doesn't inherit HasWeightCol trait. ## How was this patch tested? + [x] add an unit test. Author: Yan Facai (颜发才) <[email protected]> Closes apache#18554 from facaiy/BUG/oneVsRest_missing_weightCol. (cherry picked from commit a5a3189) Signed-off-by: Yanbo Liang <[email protected]>

The PR is related to #18554, and is modified for branch 2.1. ## What changes were proposed in this pull request? add `setWeightCol` method for OneVsRest. `weightCol` is ignored if classifier doesn't inherit HasWeightCol trait. ## How was this patch tested? + [x] add an unit test. Author: Yan Facai (颜发才) <[email protected]> Closes #18763 from facaiy/BUG/branch-2.1_OneVsRest_support_setWeightCol.

The PR is related to #18554, and is modified for branch 2.0. ## What changes were proposed in this pull request? add `setWeightCol` method for OneVsRest. `weightCol` is ignored if classifier doesn't inherit HasWeightCol trait. ## How was this patch tested? + [x] add an unit test. Author: Yan Facai (颜发才) <[email protected]> Closes #18764 from facaiy/BUG/branch-2.0_OneVsRest_support_setWeightCol.

## What changes were proposed in this pull request? add `setWeightCol` method for OneVsRest. `weightCol` is ignored if classifier doesn't inherit HasWeightCol trait. ## How was this patch tested? + [x] add an unit test. Author: Yan Facai (颜发才) <[email protected]> Closes apache#18554 from facaiy/BUG/oneVsRest_missing_weightCol. (cherry picked from commit a5a3189) Signed-off-by: Yanbo Liang <[email protected]>

The PR is related to apache#18554, and is modified for branch 2.1. add `setWeightCol` method for OneVsRest. `weightCol` is ignored if classifier doesn't inherit HasWeightCol trait. + [x] add an unit test. Author: Yan Facai (颜发才) <[email protected]> Closes apache#18763 from facaiy/BUG/branch-2.1_OneVsRest_support_setWeightCol.

facaiy added 3 commits July 6, 2017 17:38

BUG: cache weightCol if necessary

f178595

TST: add unit test

a3e6e96

TST: concise comment

931d02d

ENH: python, cache weightCol

25d681f

facaiy added 2 commits July 7, 2017 16:38

BUG: check weightCol defined

c380ba7

TST: model is not None

e511b90

CLN: use if-indent style

1c215f3

yanboliang reviewed Jul 11, 2017

View reviewed changes

facaiy added 5 commits July 13, 2017 14:05

ENH: add weightCol

2d3ce90

TST: modify test case

ea92228

ENH: add weightCol for python part

57fc4b3

CLN: revise comment

00a7ed8

TST: python test for setWeightCol

a57f096

facaiy changed the title ~~[SPARK-21306][ML] OneVsRest should cache weightCol if necessary~~ [SPARK-21306][ML] OneVsRest should support setWeightCol Jul 13, 2017

DOC: add description

54e0fca

holdenk reviewed Jul 19, 2017

View reviewed changes

TST: classifier doesn't have weightCol

9ba0e2b

yanboliang reviewed Jul 26, 2017

View reviewed changes

facaiy added 2 commits July 26, 2017 20:49

CLN: mv weightCol to the end

db303a0

TST: remove ovr2 nad ovr4

8c0beba

asfgit closed this in a5a3189 Jul 28, 2017

This was referenced Jul 28, 2017

[SPARK-21306][ML] For branch 2.1, OneVsRest should support setWeightCol #18763

Closed

[SPARK-21306][ML] For branch 2.0, OneVsRest should support setWeightCol #18764

Closed

facaiy deleted the BUG/oneVsRest_missing_weightCol branch July 28, 2017 22:37

[SPARK-21306][ML] OneVsRest should support setWeightCol #18554

[SPARK-21306][ML] OneVsRest should support setWeightCol #18554

Uh oh!

Conversation

facaiy commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

lins05 commented Jul 6, 2017

Uh oh!

facaiy commented Jul 6, 2017

Uh oh!

facaiy commented Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MLnick commented Jul 7, 2017

Uh oh!

SparkQA commented Jul 7, 2017

Uh oh!

SparkQA commented Jul 7, 2017

Uh oh!

SparkQA commented Jul 7, 2017

Uh oh!

facaiy commented Jul 11, 2017

Uh oh!

yanboliang Jul 11, 2017

Choose a reason for hiding this comment

Uh oh!

facaiy Jul 12, 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!

yanboliang Jul 12, 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!

MLnick commented Jul 11, 2017 via email

Uh oh!

MLnick commented Jul 11, 2017 via email

Uh oh!

SparkQA commented Jul 13, 2017

Uh oh!

SparkQA commented Jul 13, 2017

Uh oh!

holdenk Jul 19, 2017

Choose a reason for hiding this comment

Uh oh!

facaiy Jul 19, 2017

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 19, 2017

Uh oh!

facaiy commented Jul 26, 2017

Uh oh!

yanboliang left a comment

Choose a reason for hiding this comment

Uh oh!

yanboliang Jul 26, 2017

Choose a reason for hiding this comment

Uh oh!

facaiy Jul 26, 2017

Choose a reason for hiding this comment

Uh oh!

yanboliang Jul 26, 2017

Choose a reason for hiding this comment

Uh oh!

facaiy Jul 26, 2017

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 26, 2017

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

facaiy commented Jul 6, 2017 •

edited

Loading

facaiy commented Jul 7, 2017 •

edited

Loading

facaiy Jul 12, 2017 •

edited

Loading

yanboliang Jul 12, 2017 •

edited

Loading

yanboliang commented Jul 28, 2017 •

edited

Loading