[Minor][ML]Minor correction in the powerIterationSuite #21689
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shahidki31
changed the title
srowen
requested changes
Jul 3, 2018
| assert(localAssignments === expectedResult) | ||
|
|
||
| val predictions = Array.fill(2)(mutable.Set.empty[Long]) | ||
| assignments.select("id", "cluster").collect().foreach { |
Member
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment.
Thanks for the suggestion. I have modified.
| 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)) |
srowen
reviewed
Jul 4, 2018
| .collect() | ||
|
|
||
| val predictions = Array.fill(2)(mutable.Set.empty[Long]) | ||
| assignments.foreach{ |
Member
There was a problem hiding this comment.
This might fail the style checker -- need a space before the brace. Looks good otherwise. Let's see if the test passes.
Contributor
Author
There was a problem hiding this comment.
I have added the space. Thank you.
|
Test build #4207 has finished for PR 21689 at commit
|
srowen
approved these changes
Jul 4, 2018
Member
|
Merged to master |
Contributor
Author
|
Thank you @srowen for merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Currently the power iteration clustering test in spark 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