Skip to content

[SPARK-18625][ML] OneVsRestModel should support setFeaturesCol and setPredictionCol #16059

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
zhengruifeng wants to merge 5 commits into from
Closed

[SPARK-18625][ML] OneVsRestModel should support setFeaturesCol and setPredictionCol #16059

zhengruifeng wants to merge 5 commits into from

Conversation

@zhengruifeng
Copy link
Contributor

@zhengruifeng zhengruifeng commented Nov 29, 2016

What changes were proposed in this pull request?

add setFeaturesCol and setPredictionCol for OneVsRestModel

How was this patch tested?

added tests

@srowen
Copy link
Member

srowen commented Nov 29, 2016

Is this not the same as #15957 but for a different class?

@zhengruifeng
Copy link
Contributor Author

zhengruifeng commented Nov 29, 2016

@srowen Excepet that OvR is a meta algorithm, and we need to update the behavior of base models.

@srowen
Copy link
Member

srowen commented Nov 29, 2016

OK, should it not have been added in the other PR? I want to push back on adding logical changes piecemeal. I understand sometimes it only becomes clear later that further changes are needed, but if it happens regularly or seems avoidable it becomes a problem.

@zhengruifeng
Copy link
Contributor Author

zhengruifeng commented Nov 29, 2016

I understand your opinion that we should make same update in a batch. But I just find this issue this night...

@SparkQA
Copy link

SparkQA commented Nov 29, 2016

Test build #69322 has finished for PR 16059 at commit 4a6ecab.

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

@srowen
Copy link
Member

srowen commented Dec 2, 2016

CC @yanboliang ?

@yanboliang
Copy link
Contributor

yanboliang commented Dec 4, 2016

@srowen Thanks for cc me, I'm having a look now.

yanboliang
yanboliang reviewed Dec 4, 2016
View reviewed changes
Copy link
Contributor

@yanboliang yanboliang left a comment

Choose a reason for hiding this comment

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

Looks good overall, except some minor comments. Thanks!

mllib/src/main/scala/org/apache/spark/ml/classification/OneVsRest.scala Outdated
Copy link
Contributor

@yanboliang yanboliang Dec 4, 2016

Choose a reason for hiding this comment

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

It's not necessary to setPredictionCol here, since the prediction column of each base binary classifier is ignored during the transformation. The predictionCol of OneVsRestModel was set at L198.

mllib/src/test/scala/org/apache/spark/ml/classification/OneVsRestSuite.scala Outdated
Copy link
Contributor

@yanboliang yanboliang Dec 4, 2016

Choose a reason for hiding this comment

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

Minor: assert(outputFields === Set("y", "fea", "pred"))

@zhengruifeng
Copy link
Contributor Author

zhengruifeng commented Dec 5, 2016

@yanboliang Updated. Thanks for reviewing.

@SparkQA
Copy link

SparkQA commented Dec 5, 2016

Test build #69657 has finished for PR 16059 at commit b45f558.

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

@yanboliang
Copy link
Contributor

yanboliang commented Dec 5, 2016

LGTM, merged into master and branch-2.1. Thanks for all.

asfgit pushed a commit that referenced this pull request Dec 5, 2016
@zhengruifeng @yanboliang
[SPARK-18625][ML] OneVsRestModel should support setFeaturesCol and se…
88e07ef 
…tPredictionCol

## What changes were proposed in this pull request?
add `setFeaturesCol` and `setPredictionCol` for `OneVsRestModel`

## How was this patch tested?
added tests

Author: Zheng RuiFeng <[email protected]>

Closes #16059 from zhengruifeng/ovrm_setCol.

(cherry picked from commit bdfe7f6)
Signed-off-by: Yanbo Liang <[email protected]>
@asfgit asfgit closed this in bdfe7f6 Dec 5, 2016
@zhengruifeng zhengruifeng deleted the ovrm_setCol branch December 5, 2016 08:36
robert3005 pushed a commit to palantir/spark that referenced this pull request Dec 15, 2016
@zhengruifeng
[SPARK-18625][ML] OneVsRestModel should support setFeaturesCol and se…
df6dfb2 
…tPredictionCol

## What changes were proposed in this pull request?
add `setFeaturesCol` and `setPredictionCol` for `OneVsRestModel`

## How was this patch tested?
added tests

Author: Zheng RuiFeng <[email protected]>

Closes apache#16059 from zhengruifeng/ovrm_setCol.
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
@zhengruifeng @uzadude
[SPARK-18625][ML] OneVsRestModel should support setFeaturesCol and se…
833e479 
…tPredictionCol

## What changes were proposed in this pull request?
add `setFeaturesCol` and `setPredictionCol` for `OneVsRestModel`

## How was this patch tested?
added tests

Author: Zheng RuiFeng <[email protected]>

Closes apache#16059 from zhengruifeng/ovrm_setCol.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@yanboliang yanboliang yanboliang left review comments

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

@zhengruifeng @srowen @SparkQA @yanboliang