[SPARK-4797] Replace breezeSquaredDistance #3643

viirya · 2014-12-09T10:24:31Z

This PR replaces slow breezeSquaredDistance.

jkbradley · 2014-12-09T19:34:27Z

Hi, it looks like this may be faster for dense vectors but not for sparse. SparseVector.toArray will create a dense vector, making it much slower if the vector is very sparse. You will probably need separate cases for DenseVector and SparseVector.

viirya · 2014-12-10T06:46:14Z

Thanks. I add the consideration for different cases of SparseVector and DenseVector.

jkbradley · 2014-12-10T23:50:56Z

mllib/src/main/scala/org/apache/spark/mllib/util/MLUtils.scala

+    var squaredDistance = 0.0
+    (v1, v2) match { 
+      case (v1: SparseVector, v2: SparseVector) =>
+        v1.indices.intersect(v2.indices).foreach((idx) => {


.intersect does not know that the indices are sorted, so it may be much faster to compute the intersection explicitly. That is done in BLAS.dot; I would recommend following that pattern. That will keep you from having 3 separate iterations over the indices.

In general, I wonder if these methods would be much faster if you iterated over counters & used while loops, rather than built-in iterator methods like intersect/diff/foreach.

Thanks. I will modify this PR to follow the pattern used in BLAS.dot.

Seems that while-loop has better performance advantage than foreach. I add a commit to replace foreach with while-loop.

jkbradley · 2014-12-10T23:51:41Z

Thanks for the update! I added some comments. One more about public APIs: The new method for vectorSquaredDistance is public, which requires some careful thought about API design. I'd recommend either:
(a) making the method private, or
(b) discussing a good place for distance metrics to go + a good API for them.
If you choose (a), then a later PR could always do (b).

viirya · 2014-12-11T08:40:17Z

Thanks for that. I add new commit to make the methods private now.

viirya · 2014-12-11T12:49:50Z

Hi, intersect, diff and foreach are all replaced with while-loop in the new commit to follow BLAS.dot pattern. Please see if there is any problem. Thanks.

jkbradley · 2014-12-15T22:35:42Z

mllib/src/main/scala/org/apache/spark/mllib/util/MLUtils.scala

+        var kv1 = 0
+        var kv2 = 0
+        var score = 0.0
+        while (kv1 < nnzv1) {


Is this loop logic correct? What if v1 runs out of indices, but v2 has some left. You'll need a check for this after the loop. But a better way to write it might be to have the outer loop check kv1 < nnzv1 || kv2 < nnzv2 and handle 1 non-zero index per iteration of the loop. (That would also let you have only 1 line updating squaredDistance

jkbradley · 2014-12-15T22:36:45Z

@viirya Thanks for the updates! I made some inline comments, one of them major. Please let me know when to check again.

viirya · 2014-12-16T09:14:26Z

mllib/src/main/scala/org/apache/spark/mllib/util/MLUtils.scala

+  /**
+   * Returns the squared distance between DenseVector and SparseVector.
+   */
+  private[util] def vectorSquaredDistance(v1: SparseVector, v2: DenseVector): Double = {


Indeed. Modified in later commit.

viirya · 2014-12-16T10:09:48Z

@jkbradley Thanks. The codes are modified for your comments. The test is also expanded to test the case of the major comment you mentioned. Please check it again.

jkbradley · 2014-12-16T20:32:26Z

mllib/src/main/scala/org/apache/spark/mllib/util/MLUtils.scala

+          } else if (kv1 >= nnzv1 || (kv2 < nnzv2 && v2Indices(kv2) < v1Indices(kv1))) {
+            score = v2Values(kv2)
+            kv2 += 1
+          } else if ((kv1 < nnzv1 && kv2 < nnzv2) && v1Indices(kv1) == v2Indices(kv2)) {


No need to check (kv1 < nnzv1 && kv2 < nnzv2) here since it will always be true (because of the previous 2 if statements).

jkbradley · 2014-12-16T20:32:55Z

Just added more comments, including 1 small bug

mengxr · 2014-12-19T08:00:06Z

A high-level comment: We recently merged couple PRs that optimize linear algebra performance because breeze is slow at certain operations. But I always think it would be nice if we can optimize breeze directly unless we want to provide a full-functional linear algebra package inside mllib. If this optimization could easily go into breeze, I would suggest that direction.

viirya · 2014-12-19T10:13:22Z

Thanks @mengxr. Not quite familiar with breeze. But as I roughly go through distance metric implementations of breeze. They are following the same pattern that employs zip operation. This way can address distance metric problem in a consistent approach. But the efficiency might be not its main consideration. That seems to be matching with the goal of breeze project to be generic, clean, and powerful without sacrificing (much) efficiency. To adopt this optimization in breeze will inevitably contradict breeze's principle and design. As I can see, this optimization may not easily go into breeze. That is my rough idea.

SparkQA · 2014-12-22T21:28:57Z

Test build #552 has finished for PR 3643 at commit 91849d0.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

jkbradley · 2014-12-22T22:37:47Z

It looks like there are bugs which show up in the pyspark tests. Have you run them locally?

viirya · 2014-12-23T10:28:53Z

I have not run pyspark tests. Fixed in update.

viirya · 2014-12-23T10:30:00Z

Please test it again.

SparkQA · 2014-12-23T21:22:47Z

Test build #553 has finished for PR 3643 at commit ba34422.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2014-12-29T07:31:54Z

@jkbradley Is there any problem you concern? Is this ready to merge? Thanks.

mengxr · 2014-12-30T18:53:56Z

@viirya We recently added some linear algebra utility functions to linalg.Vectors. That should be a good place for this distance function. We use MATLAB naming convention there. Could you rebase master and move the implementation there? I suggest using sqdist as the method name and making it public.

viirya · 2014-12-31T10:30:39Z

@mengxr The implementation is renamed and moved to linalg.Vectors. Would you like to test it again?

mengxr · 2014-12-31T18:17:54Z

add to whitelist

mengxr · 2014-12-31T18:17:57Z

test this please

jkbradley · 2014-12-31T19:35:55Z

I ran some quick tests with random sparsity patterns. Averaged over 1000 iterations, it's definitely faster:

length	v1 sparsity	v2 sparsity	new time	old time	speedup
1000	1	0.5	9.42E-06	6.73E-04	71.44
1000	1	0.1	1.69E-06	5.50E-05	32.43
1000	1	0.01	1.90E-06	3.30E-05	17.40
1000	0.5	0.1	9.89E-06	7.17E-05	7.25
1000	0.5	0.01	2.54E-06	5.80E-05	22.80
1000	0.1	0.01	1.95E-06	5.82E-05	29.84
10000	1	0.5	1.11E-05	2.30E-04	20.73
10000	1	0.1	1.03E-05	2.54E-04	24.54
10000	1	0.01	8.69E-06	3.90E-04	44.92
10000	0.5	0.1	1.47E-05	3.90E-04	26.63
10000	0.5	0.01	8.63E-06	4.03E-04	46.76
10000	0.1	0.01	1.81E-06	5.96E-04	329.01
100000	1	0.5	9.27E-05	0.004039351	43.60
100000	1	0.1	9.06E-05	0.001540544	17.01
100000	1	0.01	8.71E-05	0.002636216	30.25
100000	0.5	0.1	1.15E-04	0.003777669	32.76
100000	0.5	0.01	9.61E-05	0.004879063	50.79
100000	0.1	0.01	1.89E-05	0.003148419	166.29
1000000	1	0.5	0.001017196	0.05418411	53.27

jkbradley · 2014-12-31T19:40:46Z

@viirya Thanks for the updates! LGTM pending Jenkins

SparkQA · 2014-12-31T19:47:50Z

Test build #24964 has finished for PR 3643 at commit f28b275.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

mengxr · 2014-12-31T19:52:17Z

Merged into master. Thanks! (minor TODO: Though sqdist is touched in MLUtilsSuite, it would be nice to add unit tests to VectorsSuite.)

viirya · 2015-01-01T06:04:25Z

Thanks. I will add the unit tests in a later PR soon.

Related to #3643. Follow the previous suggestion to add unit test for `sqdist` in `VectorsSuite`. Author: Liang-Chi Hsieh <[email protected]> Closes #3869 from viirya/sqdist_test and squashes the following commits: fb743da [Liang-Chi Hsieh] Modified for comment and fix bug. 90a08f3 [Liang-Chi Hsieh] Modified for comment. 39a3ca6 [Liang-Chi Hsieh] Take care of special case. b789f42 [Liang-Chi Hsieh] More proper unit test with random sparsity pattern. c36be68 [Liang-Chi Hsieh] Add unit test for sqdist.

Replace breezeSquaredDistance.

13669db

viirya changed the title ~~Replace breezeSquaredDistance~~ [SPARK-4797] Replace breezeSquaredDistance Dec 9, 2014

Consider different cases of SparseVector and DenseVector.

dd415bc

jkbradley reviewed Dec 10, 2014
View reviewed changes

Make the methods private.

d3e0628

viirya added 2 commits December 11, 2014 17:05

Use while-loop to replace foreach for better performance.

a36e09f

Follow BLAS.dot pattern to replace intersect, diff with while-loop.

f4f5ebb

jkbradley reviewed Dec 15, 2014
View reviewed changes

viirya reviewed Dec 16, 2014
View reviewed changes

Fix bug and some modifications for comments.

35db395

jkbradley reviewed Dec 16, 2014
View reviewed changes

Fix bug.

ba34422

viirya added 2 commits December 31, 2014 18:02

Merge branch 'master' into faster_squareddistance

0bc48ee

Move the implementation to linalg.Vectors and rename as sqdist.

f28b275

asfgit closed this in 06a9aa5 Dec 31, 2014

viirya mentioned this pull request Jan 1, 2015

[SPARK-5050][Mllib] Add unit test for sqdist #3869

Closed

viirya deleted the faster_squareddistance branch December 27, 2023 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-4797] Replace breezeSquaredDistance #3643

[SPARK-4797] Replace breezeSquaredDistance #3643

viirya commented Dec 9, 2014

jkbradley commented Dec 9, 2014

viirya commented Dec 10, 2014

jkbradley Dec 10, 2014

viirya Dec 11, 2014

jkbradley commented Dec 10, 2014

viirya commented Dec 11, 2014

viirya commented Dec 11, 2014

jkbradley Dec 15, 2014

jkbradley commented Dec 15, 2014

viirya Dec 16, 2014

viirya commented Dec 16, 2014

jkbradley Dec 16, 2014

jkbradley commented Dec 16, 2014

mengxr commented Dec 19, 2014

viirya commented Dec 19, 2014

SparkQA commented Dec 22, 2014

jkbradley commented Dec 22, 2014

viirya commented Dec 23, 2014

viirya commented Dec 23, 2014

SparkQA commented Dec 23, 2014

viirya commented Dec 29, 2014

mengxr commented Dec 30, 2014

viirya commented Dec 31, 2014

mengxr commented Dec 31, 2014

mengxr commented Dec 31, 2014

jkbradley commented Dec 31, 2014

jkbradley commented Dec 31, 2014

SparkQA commented Dec 31, 2014

mengxr commented Dec 31, 2014

viirya commented Jan 1, 2015

[SPARK-4797] Replace breezeSquaredDistance #3643

[SPARK-4797] Replace breezeSquaredDistance #3643

Conversation

viirya commented Dec 9, 2014

jkbradley commented Dec 9, 2014

viirya commented Dec 10, 2014

jkbradley Dec 10, 2014

Choose a reason for hiding this comment

viirya Dec 11, 2014

Choose a reason for hiding this comment

jkbradley commented Dec 10, 2014

viirya commented Dec 11, 2014

viirya commented Dec 11, 2014

jkbradley Dec 15, 2014

Choose a reason for hiding this comment

jkbradley commented Dec 15, 2014

viirya Dec 16, 2014

Choose a reason for hiding this comment

viirya commented Dec 16, 2014

jkbradley Dec 16, 2014

Choose a reason for hiding this comment

jkbradley commented Dec 16, 2014

mengxr commented Dec 19, 2014

viirya commented Dec 19, 2014

SparkQA commented Dec 22, 2014

jkbradley commented Dec 22, 2014

viirya commented Dec 23, 2014

viirya commented Dec 23, 2014

SparkQA commented Dec 23, 2014

viirya commented Dec 29, 2014

mengxr commented Dec 30, 2014

viirya commented Dec 31, 2014

mengxr commented Dec 31, 2014

mengxr commented Dec 31, 2014

jkbradley commented Dec 31, 2014

jkbradley commented Dec 31, 2014

SparkQA commented Dec 31, 2014

mengxr commented Dec 31, 2014

viirya commented Jan 1, 2015