You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by shahidki31 <gi...@git.apache.org> on 2018/07/01 19:50:36 UTC
[GitHub] spark pull request #21689: Minor correction in the powerIterationSuite
GitHub user shahidki31 opened a pull request:
https://github.com/apache/spark/pull/21689
Minor correction in the powerIterationSuite
## What changes were proposed in this pull request?
Currently the power iteration clustering test in ml maps the results to the labels 0 and 1 for assertion. Since the clustering outputs need not be the same as the mapped labels, it may cause failure in the test case.
Even if it correctly maps, theoretically we cannot guarantee which set belongs to which cluster label. KMeans can assign label 0 to either of the set.
PowerIterationClusteringSuite in the MLLib checks the clustering results without mapping to the particular cluster label, as shown below.
`` val predictions = Array.fill(2)(mutable.Set.empty[Long])
model.assignments.collect().foreach { a =>
predictions(a.cluster) += a.id
}
assert(predictions.toSet == Set((0 until n1).toSet, (n1 until n).toSet))
``
## How was this patch tested?
Existing tests
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shahidki31/spark picTestSuiteMinorCorrection
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/21689.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #21689
----
commit 7b52f1ebbd4b7afd088c41695c61f4475911271e
Author: Shahid <sh...@...>
Date: 2018-07-01T19:39:19Z
Minor correction in the powerIterationSuite
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21689: [Minor][ML]Minor correction in the powerIteration...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/21689#discussion_r200131855
--- Diff: mllib/src/test/scala/org/apache/spark/ml/clustering/PowerIterationClusteringSuite.scala ---
@@ -76,23 +78,31 @@ class PowerIterationClusteringSuite extends SparkFunSuite
.setMaxIter(40)
.setWeightCol("weight")
.assignClusters(data)
- val localAssignments = assignments
- .select('id, 'cluster)
- .as[(Long, Int)].collect().toSet
- val expectedResult = (0 until n1).map(x => (x, 1)).toSet ++
- (n1 until n).map(x => (x, 0)).toSet
- assert(localAssignments === expectedResult)
+ .select("id", "cluster")
+ .as[(Long, Int)]
+ .collect()
+
+ val predictions = Array.fill(2)(mutable.Set.empty[Long])
+ assignments.foreach{
--- End diff --
This might fail the style checker -- need a space before the brace. Looks good otherwise. Let's see if the test passes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21689: [Minor][ML]Minor correction in the powerIteration...
Posted by shahidki31 <gi...@git.apache.org>.
Github user shahidki31 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21689#discussion_r200136002
--- Diff: mllib/src/test/scala/org/apache/spark/ml/clustering/PowerIterationClusteringSuite.scala ---
@@ -76,23 +78,31 @@ class PowerIterationClusteringSuite extends SparkFunSuite
.setMaxIter(40)
.setWeightCol("weight")
.assignClusters(data)
- val localAssignments = assignments
- .select('id, 'cluster)
- .as[(Long, Int)].collect().toSet
- val expectedResult = (0 until n1).map(x => (x, 1)).toSet ++
- (n1 until n).map(x => (x, 0)).toSet
- assert(localAssignments === expectedResult)
+ .select("id", "cluster")
+ .as[(Long, Int)]
+ .collect()
+
+ val predictions = Array.fill(2)(mutable.Set.empty[Long])
+ assignments.foreach{
--- End diff --
I have added the space. Thank you.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21689: [Minor][ML]Minor correction in the powerIterationSuite
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21689
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21689: [Minor][ML]Minor correction in the powerIterationSuite
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/21689
Merged to master
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21689: [Minor][ML]Minor correction in the powerIteration...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/21689#discussion_r199950628
--- Diff: mllib/src/test/scala/org/apache/spark/ml/clustering/PowerIterationClusteringSuite.scala ---
@@ -76,23 +78,25 @@ class PowerIterationClusteringSuite extends SparkFunSuite
.setMaxIter(40)
.setWeightCol("weight")
.assignClusters(data)
- val localAssignments = assignments
- .select('id, 'cluster)
- .as[(Long, Int)].collect().toSet
- val expectedResult = (0 until n1).map(x => (x, 1)).toSet ++
- (n1 until n).map(x => (x, 0)).toSet
- assert(localAssignments === expectedResult)
+
+ val predictions = Array.fill(2)(mutable.Set.empty[Long])
+ assignments.select("id", "cluster").collect().foreach {
--- End diff --
Nit: I think this was clearer with `.as[(Long,Int)]` as it avoids matching `Row`. I don't feel strongly about it; just less change.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21689: [Minor][ML]Minor correction in the powerIterationSuite
Posted by shahidki31 <gi...@git.apache.org>.
Github user shahidki31 commented on the issue:
https://github.com/apache/spark/pull/21689
Hi, @srowen . I have modified the code based on your suggestions.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21689: [Minor][ML]Minor correction in the powerIterationSuite
Posted by shahidki31 <gi...@git.apache.org>.
Github user shahidki31 commented on the issue:
https://github.com/apache/spark/pull/21689
Thank you @srowen for merging.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21689: [Minor][ML]Minor correction in the powerIterationSuite
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21689
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21689: [Minor][ML]Minor correction in the powerIteration...
Posted by shahidki31 <gi...@git.apache.org>.
Github user shahidki31 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21689#discussion_r200040108
--- Diff: mllib/src/test/scala/org/apache/spark/ml/clustering/PowerIterationClusteringSuite.scala ---
@@ -76,23 +78,25 @@ class PowerIterationClusteringSuite extends SparkFunSuite
.setMaxIter(40)
.setWeightCol("weight")
.assignClusters(data)
- val localAssignments = assignments
- .select('id, 'cluster)
- .as[(Long, Int)].collect().toSet
- val expectedResult = (0 until n1).map(x => (x, 1)).toSet ++
- (n1 until n).map(x => (x, 0)).toSet
- assert(localAssignments === expectedResult)
+
+ val predictions = Array.fill(2)(mutable.Set.empty[Long])
+ assignments.select("id", "cluster").collect().foreach {
+ case Row(id: Long, cluster: Integer) => predictions(cluster) += id
+ }
+ assert(predictions.toSet == Set((0 until n1).toSet, (n1 until n).toSet))
--- End diff --
Yes. done.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21689: [Minor][ML]Minor correction in the powerIterationSuite
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21689
**[Test build #4207 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4207/testReport)** for PR 21689 at commit [`b02cae5`](https://github.com/apache/spark/commit/b02cae5d3b735ea5c281fe125bf4b5792940294b).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21689: [Minor][ML]Minor correction in the powerIterationSuite
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21689
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21689: [Minor][ML]Minor correction in the powerIteration...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/21689
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21689: [Minor][ML]Minor correction in the powerIteration...
Posted by shahidki31 <gi...@git.apache.org>.
Github user shahidki31 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21689#discussion_r200039891
--- Diff: mllib/src/test/scala/org/apache/spark/ml/clustering/PowerIterationClusteringSuite.scala ---
@@ -76,23 +78,25 @@ class PowerIterationClusteringSuite extends SparkFunSuite
.setMaxIter(40)
.setWeightCol("weight")
.assignClusters(data)
- val localAssignments = assignments
- .select('id, 'cluster)
- .as[(Long, Int)].collect().toSet
- val expectedResult = (0 until n1).map(x => (x, 1)).toSet ++
- (n1 until n).map(x => (x, 0)).toSet
- assert(localAssignments === expectedResult)
+
+ val predictions = Array.fill(2)(mutable.Set.empty[Long])
+ assignments.select("id", "cluster").collect().foreach {
--- End diff --
Thanks for the suggestion. I have modified.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21689: [Minor][ML]Minor correction in the powerIterationSuite
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21689
**[Test build #4207 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4207/testReport)** for PR 21689 at commit [`b02cae5`](https://github.com/apache/spark/commit/b02cae5d3b735ea5c281fe125bf4b5792940294b).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21689: [Minor][ML]Minor correction in the powerIteration...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/21689#discussion_r199950647
--- Diff: mllib/src/test/scala/org/apache/spark/ml/clustering/PowerIterationClusteringSuite.scala ---
@@ -76,23 +78,25 @@ class PowerIterationClusteringSuite extends SparkFunSuite
.setMaxIter(40)
.setWeightCol("weight")
.assignClusters(data)
- val localAssignments = assignments
- .select('id, 'cluster)
- .as[(Long, Int)].collect().toSet
- val expectedResult = (0 until n1).map(x => (x, 1)).toSet ++
- (n1 until n).map(x => (x, 0)).toSet
- assert(localAssignments === expectedResult)
+
+ val predictions = Array.fill(2)(mutable.Set.empty[Long])
+ assignments.select("id", "cluster").collect().foreach {
+ case Row(id: Long, cluster: Integer) => predictions(cluster) += id
+ }
+ assert(predictions.toSet == Set((0 until n1).toSet, (n1 until n).toSet))
--- End diff --
I think we want `===` here?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org