-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-5050][Mllib] Add unit test for sqdist #3869
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.
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
Conversation
|
Test build #24985 has finished for PR 3869 at commit
|
|
@viirya Could you please add the "[mllib]" tag to the title? |
| val v4 = Vectors.sparse(n, indices.slice(0, m - 10), | ||
| indices.map(i => a(i) + 0.5).slice(0, m - 10)) | ||
| val squaredDist = breezeSquaredDistance(v2.toBreeze, v4.toBreeze) | ||
| val fastSquaredDist = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can fit on 1 line instead of 2
|
@viirya Looks OK to me, except for the tiny comments. Thanks! At some point, it might be nice to replace these tests with ones using random dense & sparse vectors (with random sparsity patterns). If you are interested in doing that, I can send you a method for generating random sparse vectors which I used for the timing tests. |
|
@jkbradley Thanks. I made a more proper unit test with random sparsity pattern. |
|
Test build #25008 has finished for PR 3869 at commit
|
|
Test build #25009 has finished for PR 3869 at commit
|
|
(I meant those comments for the main text but accidentally added them to the commit instead.) |
|
Other than those small comments, it looks good. Thanks! |
|
Test build #25026 has finished for PR 3869 at commit
|
| @@ -175,6 +177,33 @@ class VectorsSuite extends FunSuite { | |||
| assert(v.size === x.rows) | |||
| } | |||
|
|
|||
| test("sqdist") { | |||
| val random = new Random(System.nanoTime()) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new Random uses a random seed already.
|
Test build #25096 has finished for PR 3869 at commit
|
|
LGTM. Merged into master. Thanks! |
Related to #3643. Follow the previous suggestion to add unit test for
sqdistinVectorsSuite.