Uh oh!
There was an error while loading. Please reload this page.
[SPARK-16473][MLLIB] Fix BisectingKMeans Algorithm failing in edge case - #16355
[SPARK-16473][MLLIB] Fix BisectingKMeans Algorithm failing in edge case#16355imatiach-msft wants to merge 7 commits into
Conversation
…se where no children exist in updateAssignments
wangmiao1981
commented
Dec 21, 2016
Jenkins, test this please. |
wangmiao1981
commented
Dec 21, 2016
@imatiach-msft Can you add a test case? |
alokob
commented
Dec 21, 2016
@imatiach-msft , thanks for creating pull request and committing change which I have shared , I will try to share some sample dataset for this issue. |
| val children = Seq(leftChildIndex(index), rightChildIndex(index)) | ||
| val selected = children.minBy { child => | ||
| KMeans.fastSquaredDistance(newClusterCenters(child), v) | ||
| if (children.length > 0) { |
There was a problem hiding this comment.
Nit: you could write children.nonEmpty, but, when would it not have length 2? it's initialized in the line above.
There was a problem hiding this comment.
I can see following possibility to get java.util.NoSuchElementException in the original code.
leftChildIndex(index) , this methods returns index2 and rightChildIndex(index) returns index2+1. If the Map object newClusterCenters does not have entries belonging to leftChildIndex(index) or rightChildIndex(index) .
Here we need to have this check newClusterCenters.contains(child) , to avoid this issue.
There was a problem hiding this comment.
I added a filter on whether new cluster centers contains the children. Would you be able to send me a sample dataset so I can validate the fix? Thank you!
imatiach-msft
commented
Dec 21, 2016
Good point. It looks like we should be checking if the map contains the child or not. However, I'm not sure if that is the correct solution either. I need a repro dataset from the bug reporter. |
srowen
commented
Dec 21, 2016
That makes more sense as a fix, yes. Sounds like there is still a to-do to verify the fix. If it's possible to write a simple unit test to cover it, all the better. |
imatiach-msft
commented
Dec 22, 2016
Yep, there is still a TODO to verify the fix. I'm waiting for the dataset from Alok to reproduce the issue: |
alokob
commented
Dec 23, 2016
You can get sample vectors at this location https://github.com/alokob/SparkClusteringDataSet/SampleVectors.txt. Also while executing bisecting K-Means , we have set following configuration settings , BisectingKMeans.setK(100); Please let me know if any additional details are needed. |
alokob
commented
Dec 26, 2016
@imatiach-msft Did you find the dataset suitable. Is anything else needed from my side? |
imatiach-msft
commented
Dec 26, 2016
Hi Alok! |
alokob
commented
Dec 28, 2016
Thats ok , enjoy Xmas. |
imatiach-msft
commented
Dec 28, 2016
I have very good news :). I was not only able to repro the issue with your dataset, but I was also able to verify that with the suggested fix the algorithm does not fail (adding the val newClusterChildren = children.filter(newClusterCenters.contains(_)) fixed the issue). I need to figure out how to actually add the test case to spark though - my understanding is that checking in dataset files is not allowed? My test code (not cleaned up yet) was: import org.apache.spark.mllib.linalg.{ import org.apache.spark.ml.linalg.{ @transient var loadedDataset: Dataset[_] = _ override def beforeAll(): Unit = { } |
jkbradley
commented
Dec 28, 2016
ok to test |
alokob
commented
Dec 28, 2016
Nice to know that , codefix I suggested is working. Its really nice to contribute in spark. |
SparkQA
commented
Dec 28, 2016
Test build #70679 has finished for PR 16355 at commit
|
…c sparse data which can generate the exception user encountered.
imatiach-msft
commented
Dec 28, 2016
I've updated with a new commit. I was able to reproduce the issue by generating a synthetic sparse dataset similar to the one Alok sent me, in accordance with the test-style of spark test methods. Hence, I was able to verify the fix. Please review the new code changes and let me know who else needs to review in order for the changes to be committed. |
imatiach-msft
commented
Dec 28, 2016
Jenkins, retest this please |
SparkQA
commented
Dec 28, 2016
Test build #70682 has finished for PR 16355 at commit
|
imatiach-msft
commented
Dec 28, 2016
Jenkins, retest this please |
SparkQA
commented
Dec 29, 2016
Test build #70688 has finished for PR 16355 at commit
|
imatiach-msft
commented
Dec 29, 2016
@jkbradley@srowen any comments on the changes? Thank you! |
jkbradley
commented
Dec 29, 2016
@yu-iskw Pinging on this since you wrote bisecting k-means originally. Do you have time to take a look? Thanks! |
the only problem I see is that with this code we generate k-1 clusters instead of k, but it states in the algorithm documentation that it is not guaranteed to generate k clusters, it could be fewer if the leaf clusters are not divisible (see spark/mllib/src/main/scala/org/apache/spark/mllib/clustering/BisectingKMeans.scala): Iteratively it finds divisible clusters on the bottom level and bisects each of them using It seems in the dataset Alok gave, one of the clusters which was assumed to be divisible and was divided ended up generating two clusters, one which contained all the points and the other none, which is what created the error (his cluster 162, child of 81, was empty, but cluster 163 was non-empty after reassignment). My intuition is that it should be possible for the algorithm to account for failed splits and then try to split in a different leaf node, but that change seems non-trivial to me. |
jkbradley
left a comment
There was a problem hiding this comment.
Thanks for the updates! I agree we shouldn't try to retry failed splits in this PR.
| override def beforeAll(): Unit = { | ||
| super.beforeAll() | ||
| dataset = KMeansSuite.generateKMeansData(spark, 50, 3, k) | ||
| sparseDataset = KMeansSuite.generateSparseData(spark, 100, 1000, k, 42) |
There was a problem hiding this comment.
Does the test really need to be this large? It takes ~1 sec which is long-ish for a unit test.
There was a problem hiding this comment.
I can only repro the issue with very sparse data where number of columns is around 1k. I was able to reduce the number of rows to be only 10 however. I hope that is a small enough dataset.
| val bkm = new BisectingKMeans().setK(k).setMinDivisibleClusterSize(4).setMaxIter(4) | ||
| assert(bkm.getK === k) | ||
| assert(bkm.getFeaturesCol === "features") |
There was a problem hiding this comment.
There's no need to test featuresCol, predictionCol, and other things which aren't relevant to this unit test. I'd simplify it.
| assert(bkm.getPredictionCol === "prediction") | ||
| assert(bkm.getMaxIter === 4) | ||
| assert(bkm.getMinDivisibleClusterSize === 4) | ||
| // Verify fit does not fail on very sparse data |
There was a problem hiding this comment.
It's not clear to me that this unit test actually tests the issue fixed in this PR. Is there a good way to see why it would? If not, then it would be great to write a tiny dataset by hand which would trigger the failure.
There was a problem hiding this comment.
I added this check to verify:
// Verify we hit the edge case
assert(numClusters < k && numClusters > 1)
the issue only occurs for very sparse data, but it occurs very consistently (almost all very sparse data that I generate can trigger the error)
| spark.createDataFrame(rdd) | ||
| } | ||
| def generateSparseData(spark: SparkSession, rows: Int, dim: Int, k: Int, seed: Int): DataFrame = { |
There was a problem hiding this comment.
modified the method to use it
jkbradley
left a comment
There was a problem hiding this comment.
Just 2 small comments left. Thanks!
| "one cluster is empty after split") { | ||
| val bkm = new BisectingKMeans().setK(k).setMinDivisibleClusterSize(4).setMaxIter(4) | ||
| assert(bkm.getK === k) |
There was a problem hiding this comment.
Don't bother testing getK, getMaxIter, or getMinDivisibleClusterSize
The setters should be tested elsewhere, so you may assume they work here.
| val nnz = random.nextInt(dim) | ||
| val rdd = sc.parallelize(1 to rows) | ||
| .map(i => Vectors.sparse(dim, random.shuffle(0 to dim - 1).slice(0, nnz).sorted.toArray, | ||
| Array.fill(nnz)(random.nextInt(k).toDouble))) |
There was a problem hiding this comment.
I don't understand this use of k. The feature value can be any random number. I'd remove k.
imatiach-msft
commented
Jan 13, 2017
@jkbradley thanks, I've updated the code based on your latest comments - I removed k and the verification for the setters. |
SparkQA
commented
Jan 13, 2017
Test build #71289 has finished for PR 16355 at commit
|
imatiach-msft
commented
Jan 17, 2017
ping @jkbradley would you be able to take another look at the bisecting K-Means fix? |
jkbradley
commented
Jan 17, 2017
test this please |
| test("SPARK-16473: Verify Bisecting K-Means does not fail in edge case where" + | ||
| "one cluster is empty after split") { | ||
| val bkm = new BisectingKMeans().setK(k).setMinDivisibleClusterSize(4).setMaxIter(4) |
There was a problem hiding this comment.
Please set the seed too since this test seems at risk of flakiness
jkbradley
commented
Jan 17, 2017
I was about to say this is ready, but I do think we should add the seed. Other than that, this should be ready! |
imatiach-msft
commented
Jan 17, 2017
@jkbradley done, added seed. Thanks! |
SparkQA
commented
Jan 17, 2017
Test build #71533 has finished for PR 16355 at commit
|
SparkQA
commented
Jan 18, 2017
Test build #71538 has finished for PR 16355 at commit
|
SparkQA
commented
Jan 18, 2017
Test build #3538 has finished for PR 16355 at commit
|
imatiach-msft
commented
Jan 18, 2017
ping @jkbradley would you be able to take another look at the bisecting kmeans model? I've updated with the random seed as requested. |
imatiach-msft
commented
Jan 18, 2017
ping @jkbradley would you be able to take another look at the bisecting kmeans model? I've updated with the random seed as requested, and the build succeeded. Thank you! |
imatiach-msft
commented
Jan 20, 2017
ping @jkbradley would you be able to take another look at the bisecting kmeans model? Thanks! |
imatiach-msft
commented
Jan 23, 2017
ping @jkbradley would you be able to take another look at the bisecting kmeans model? Thanks! |
jkbradley
commented
Jan 23, 2017
LGTM |
SparkQA
commented
Jan 23, 2017
Test build #3548 has finished for PR 16355 at commit
|
jkbradley
commented
Jan 23, 2017
Merging with master. Will try to backport to branch-2.1 as well. |
jkbradley
commented
Jan 23, 2017
I was able to check out this commit and test it with branch-2.1, but now I can't get the merge script to merge it for branch-2.1. @srowen would you mind trying? Thanks! |
vanzin
commented
Jan 23, 2017
I just had some issues with that too. But manually merging (git cherry-pick + git push) seems to still work, so maybe try that. |
srowen
commented
Jan 24, 2017
It's an apache-github sync issue: https://github.com/apache/spark/commits/branch-2.1 I'll cherry-pick onto apache/branch-2.1 and push as that might also kick the sync to try again. |
[SPARK-16473][MLLIB] Fix BisectingKMeans Algorithm failing in edge case where no children exist in updateAssignments ## What changes were proposed in this pull request? Fix a bug in which BisectingKMeans fails with error: java.util.NoSuchElementException: key not found: 166 at scala.collection.MapLike$class.default(MapLike.scala:228) at scala.collection.AbstractMap.default(Map.scala:58) at scala.collection.MapLike$class.apply(MapLike.scala:141) at scala.collection.AbstractMap.apply(Map.scala:58) at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1$$anonfun$2.apply$mcDJ$sp(BisectingKMeans.scala:338) at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1$$anonfun$2.apply(BisectingKMeans.scala:337) at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1$$anonfun$2.apply(BisectingKMeans.scala:337) at scala.collection.TraversableOnce$$anonfun$minBy$1.apply(TraversableOnce.scala:231) at scala.collection.LinearSeqOptimized$class.foldLeft(LinearSeqOptimized.scala:111) at scala.collection.immutable.List.foldLeft(List.scala:84) at scala.collection.LinearSeqOptimized$class.reduceLeft(LinearSeqOptimized.scala:125) at scala.collection.immutable.List.reduceLeft(List.scala:84) at scala.collection.TraversableOnce$class.minBy(TraversableOnce.scala:231) at scala.collection.AbstractTraversable.minBy(Traversable.scala:105) at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1.apply(BisectingKMeans.scala:337) at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1.apply(BisectingKMeans.scala:334) at scala.collection.Iterator$$anon$11.next(Iterator.scala:328) at scala.collection.Iterator$$anon$14.hasNext(Iterator.scala:389) ## How was this patch tested? The dataset was run against the code change to verify that the code works. I will try to add unit tests to the code. (Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests) (If this patch involves UI changes, please attach a screenshot; otherwise, remove this) Please review http://spark.apache.org/contributing.html before opening a pull request. Author: Ilya Matiach <ilmat@microsoft.com> Closes#16355 from imatiach-msft/ilmat/fix-kmeans.
srowen
commented
Jan 24, 2017
Done, and it synced now. Merged to master/2.1 |
jkbradley
commented
Jan 24, 2017
Oh OK! Thanks @srowen |
[SPARK-16473][MLLIB] Fix BisectingKMeans Algorithm failing in edge case where no children exist in updateAssignments ## What changes were proposed in this pull request? Fix a bug in which BisectingKMeans fails with error: java.util.NoSuchElementException: key not found: 166 at scala.collection.MapLike$class.default(MapLike.scala:228) at scala.collection.AbstractMap.default(Map.scala:58) at scala.collection.MapLike$class.apply(MapLike.scala:141) at scala.collection.AbstractMap.apply(Map.scala:58) at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1$$anonfun$2.apply$mcDJ$sp(BisectingKMeans.scala:338) at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1$$anonfun$2.apply(BisectingKMeans.scala:337) at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1$$anonfun$2.apply(BisectingKMeans.scala:337) at scala.collection.TraversableOnce$$anonfun$minBy$1.apply(TraversableOnce.scala:231) at scala.collection.LinearSeqOptimized$class.foldLeft(LinearSeqOptimized.scala:111) at scala.collection.immutable.List.foldLeft(List.scala:84) at scala.collection.LinearSeqOptimized$class.reduceLeft(LinearSeqOptimized.scala:125) at scala.collection.immutable.List.reduceLeft(List.scala:84) at scala.collection.TraversableOnce$class.minBy(TraversableOnce.scala:231) at scala.collection.AbstractTraversable.minBy(Traversable.scala:105) at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1.apply(BisectingKMeans.scala:337) at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1.apply(BisectingKMeans.scala:334) at scala.collection.Iterator$$anon$11.next(Iterator.scala:328) at scala.collection.Iterator$$anon$14.hasNext(Iterator.scala:389) ## How was this patch tested? The dataset was run against the code change to verify that the code works. I will try to add unit tests to the code. (Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests) (If this patch involves UI changes, please attach a screenshot; otherwise, remove this) Please review http://spark.apache.org/contributing.html before opening a pull request. Author: Ilya Matiach <ilmat@microsoft.com> Closesapache#16355 from imatiach-msft/ilmat/fix-kmeans.
[SPARK-16473][MLLIB] Fix BisectingKMeans Algorithm failing in edge case where no children exist in updateAssignments ## What changes were proposed in this pull request? Fix a bug in which BisectingKMeans fails with error: java.util.NoSuchElementException: key not found: 166 at scala.collection.MapLike$class.default(MapLike.scala:228) at scala.collection.AbstractMap.default(Map.scala:58) at scala.collection.MapLike$class.apply(MapLike.scala:141) at scala.collection.AbstractMap.apply(Map.scala:58) at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1$$anonfun$2.apply$mcDJ$sp(BisectingKMeans.scala:338) at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1$$anonfun$2.apply(BisectingKMeans.scala:337) at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1$$anonfun$2.apply(BisectingKMeans.scala:337) at scala.collection.TraversableOnce$$anonfun$minBy$1.apply(TraversableOnce.scala:231) at scala.collection.LinearSeqOptimized$class.foldLeft(LinearSeqOptimized.scala:111) at scala.collection.immutable.List.foldLeft(List.scala:84) at scala.collection.LinearSeqOptimized$class.reduceLeft(LinearSeqOptimized.scala:125) at scala.collection.immutable.List.reduceLeft(List.scala:84) at scala.collection.TraversableOnce$class.minBy(TraversableOnce.scala:231) at scala.collection.AbstractTraversable.minBy(Traversable.scala:105) at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1.apply(BisectingKMeans.scala:337) at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1.apply(BisectingKMeans.scala:334) at scala.collection.Iterator$$anon$11.next(Iterator.scala:328) at scala.collection.Iterator$$anon$14.hasNext(Iterator.scala:389) ## How was this patch tested? The dataset was run against the code change to verify that the code works. I will try to add unit tests to the code. (Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests) (If this patch involves UI changes, please attach a screenshot; otherwise, remove this) Please review http://spark.apache.org/contributing.html before opening a pull request. Author: Ilya Matiach <ilmat@microsoft.com> Closesapache#16355 from imatiach-msft/ilmat/fix-kmeans.
[SPARK-16473][MLLIB] Fix BisectingKMeans Algorithm failing in edge case where no children exist in updateAssignments
What changes were proposed in this pull request?
Fix a bug in which BisectingKMeans fails with error:
java.util.NoSuchElementException: key not found: 166
at scala.collection.MapLike$class.default(MapLike.scala:228)
at scala.collection.AbstractMap.default(Map.scala:58)
at scala.collection.MapLike$class.apply(MapLike.scala:141)
at scala.collection.AbstractMap.apply(Map.scala:58)
at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1$$anonfun$2.apply$mcDJ$sp(BisectingKMeans.scala:338)
at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1$$anonfun$2.apply(BisectingKMeans.scala:337)
at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1$$anonfun$2.apply(BisectingKMeans.scala:337)
at scala.collection.TraversableOnce$$anonfun$minBy$1.apply(TraversableOnce.scala:231)
at scala.collection.LinearSeqOptimized$class.foldLeft(LinearSeqOptimized.scala:111)
at scala.collection.immutable.List.foldLeft(List.scala:84)
at scala.collection.LinearSeqOptimized$class.reduceLeft(LinearSeqOptimized.scala:125)
at scala.collection.immutable.List.reduceLeft(List.scala:84)
at scala.collection.TraversableOnce$class.minBy(TraversableOnce.scala:231)
at scala.collection.AbstractTraversable.minBy(Traversable.scala:105)
at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1.apply(BisectingKMeans.scala:337)
at org.apache.spark.mllib.clustering.BisectingKMeans$$anonfun$org$apache$spark$mllib$clustering$BisectingKMeans$$updateAssignments$1.apply(BisectingKMeans.scala:334)
at scala.collection.Iterator$$anon$11.next(Iterator.scala:328)
at scala.collection.Iterator$$anon$14.hasNext(Iterator.scala:389)
How was this patch tested?
The dataset was run against the code change to verify that the code works. I will try to add unit tests to the code.
(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)
Please review http://spark.apache.org/contributing.html before opening a pull request.