[SPARK-14634][ML] Add BisectingKMeansSummary #12394
Conversation
|
Test build #55820 has finished for PR 12394 at commit
|
|
cc @jkbradley |
|
This looks good. |
|
cc @jkbradley @mengxr Summary for BisectingKMeans is still missing now |
|
Test build #58702 has finished for PR 12394 at commit
|
|
cc @mengxr |
zhengruifeng
force-pushed
the
biKMSummary
branch
from
7b8f308 to
64ce1a7
Compare
|
Test build #66156 has finished for PR 12394 at commit
|
|
@yanboliang Could you please have a review? |
|
Jenkins, test this please. |
|
Test build #66958 has finished for PR 12394 at commit
|
|
LGTM, merged into master. Thanks! |
jkbradley
left a comment
jkbradley
left a comment
There was a problem hiding this comment.
Just saw this. Sorry to come late to the discussion.
This looks good, but I realized I messed up when reviewing the similar KMeansSummary for 2.0: I should have separated a Summary from a TrainingSummary. Maybe it's not important for clustering, so no need to change this PR. But in the future, for other models, let's try to separate the 2. It's really important for linear models, and it might become more important for others as well. Thanks!
@zhengruifeng @yanboliang does that sound good?
| val model = copyValues(new BisectingKMeansModel(uid, parentModel).setParent(this)) | ||
| val summary = new BisectingKMeansSummary( | ||
| model.transform(dataset), $(predictionCol), $(featuresCol), $(k)) | ||
| model.setSummary(summary) |
There was a problem hiding this comment.
This looks like a superfluous call.
There was a problem hiding this comment.
yes, I will del this line ASAP. Thanks for your comment!
|
@jkbradley Sounds good to me! |
|
@jkbradley +1 Sounds good. |
## What changes were proposed in this pull request? As commented by jkbradley in apache#12394, `model.setSummary(summary)` is superfluous ## How was this patch tested? existing tests Author: Zheng RuiFeng <[email protected]> Closes apache#15619 from zhengruifeng/del_superfluous.
## What changes were proposed in this pull request? Add BisectingKMeansSummary ## How was this patch tested? unit test Author: Zheng RuiFeng <[email protected]> Closes apache#12394 from zhengruifeng/biKMSummary.
## What changes were proposed in this pull request? As commented by jkbradley in apache#12394, `model.setSummary(summary)` is superfluous ## How was this patch tested? existing tests Author: Zheng RuiFeng <[email protected]> Closes apache#15619 from zhengruifeng/del_superfluous.
## What changes were proposed in this pull request? Add BisectingKMeansSummary ## How was this patch tested? unit test Author: Zheng RuiFeng <[email protected]> Closes apache#12394 from zhengruifeng/biKMSummary.
## What changes were proposed in this pull request? As commented by jkbradley in apache#12394, `model.setSummary(summary)` is superfluous ## How was this patch tested? existing tests Author: Zheng RuiFeng <[email protected]> Closes apache#15619 from zhengruifeng/del_superfluous.
4 participants
What changes were proposed in this pull request?
Add BisectingKMeansSummary
How was this patch tested?
unit test