You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by wilson-lauw <gi...@git.apache.org> on 2016/07/15 08:35:51 UTC
[GitHub] spark pull request #14219: sort sparseVector's indices before doing multipli...
GitHub user wilson-lauw opened a pull request:
https://github.com/apache/spark/pull/14219
sort sparseVector's indices before doing multiplication
## What changes were proposed in this pull request?
sort sparseVector's indices before doing multiplication to make sure the result returned correctly
## How was this patch tested?
manual and existing tests
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/wilson-lauw/spark SPARK-16566
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/14219.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 #14219
----
commit 2aaf742dac7e8d01387d0f615e3b951a2bf5b1d4
Author: Wilson Lauw <wi...@salestock.id>
Date: 2016-07-15T08:34:31Z
sort sparseVector's indices before doing multiplication
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14219: [SPARK-16566][MLLib] sort sparseVector's indices ...
Posted by wilson-lauw <gi...@git.apache.org>.
Github user wilson-lauw commented on a diff in the pull request:
https://github.com/apache/spark/pull/14219#discussion_r71086805
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/linalg/BLAS.scala ---
@@ -617,8 +617,27 @@ private[spark] object BLAS extends Serializable with Logging {
x: SparseVector,
beta: Double,
y: DenseVector): Unit = {
- val xValues = x.values
- val xIndices = x.indices
+
+ def isSorted(l:Array[Int]): Boolean = {
--- End diff --
Thanks for the suggestion, will fix it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14219: [SPARK-16566][MLLib] sort sparseVector's indices before ...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on the issue:
https://github.com/apache/spark/pull/14219
SparseVector indices are assumed to be sorted already. We should not re-sort them during operations. The deeper issue which needs to be fixed is SPARK-14707
I commented in the JIRA as well.
Could you please close this issue?
Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14219: [SPARK-16566][MLLib] sort sparseVector's indices before ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14219
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14219: [SPARK-16566][MLLib] sort sparseVector's indices before ...
Posted by wilson-lauw <gi...@git.apache.org>.
Github user wilson-lauw commented on the issue:
https://github.com/apache/spark/pull/14219
@hhbyyh I just checked, the implementation of `def dot(x: SparseVector, y: SparseVector)` also need to be modified.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14219: [SPARK-16566][MLLib] sort sparseVector's indices before ...
Posted by wilson-lauw <gi...@git.apache.org>.
Github user wilson-lauw commented on the issue:
https://github.com/apache/spark/pull/14219
Closing this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14219: [SPARK-16566][MLLib] sort sparseVector's indices before ...
Posted by hhbyyh <gi...@git.apache.org>.
Github user hhbyyh commented on the issue:
https://github.com/apache/spark/pull/14219
Order of the indices in SparseVector potentially affects many other BLAS operations. Perhaps we should have it in a more general/reusable pattern if we do want to impose the validation.
Yet the most efficient way as I see is still to perform the check during construction, maybe something like an optional `validate` parameter.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14219: [SPARK-16566][MLLib] sort sparseVector's indices before ...
Posted by wilson-lauw <gi...@git.apache.org>.
Github user wilson-lauw commented on the issue:
https://github.com/apache/spark/pull/14219
file org.apache.spark.ml.linalg.BLAS.scala on module mllib-local also need to be fixed, once the changes is okay, I will also apply the changes there.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14219: [SPARK-16566][MLLib] sort sparseVector's indices ...
Posted by wilson-lauw <gi...@git.apache.org>.
Github user wilson-lauw closed the pull request at:
https://github.com/apache/spark/pull/14219
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14219: [SPARK-16566][MLLib] sort sparseVector's indices ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/14219#discussion_r71077975
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/linalg/BLAS.scala ---
@@ -617,8 +617,27 @@ private[spark] object BLAS extends Serializable with Logging {
x: SparseVector,
beta: Double,
y: DenseVector): Unit = {
- val xValues = x.values
- val xIndices = x.indices
+
+ def isSorted(l:Array[Int]): Boolean = {
+ var index = 1
+ while(index < l.length){
+ val prevElement = l(index - 1)
+ val currentElement = l(index)
+
+ if(prevElement > currentElement) return false
+ index += 1
+ }
+ return true
+ }
+
+ val (xValues,xIndices) = isSorted(x.indices) match{
--- End diff --
match is overkilll for a boolean. Just use an `if`. We should avoid the style that omits member dereference "." here. Also you can use `unzip` I think
```
val (xValues, xIndices) =
if (isSorted(x.indices)) {
(x.values, x.indices)
} else {
x.values.zip(x.indices).sortBy(_._2).unzip
}
```
The `def isSorted` probably should just be inlined, even, or made non-local
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14219: [SPARK-16566][MLLib] sort sparseVector's indices before ...
Posted by wilson-lauw <gi...@git.apache.org>.
Github user wilson-lauw commented on the issue:
https://github.com/apache/spark/pull/14219
Modifications also needed on module mllib-local, file org.apache.spark.ml.linalg.Vectors.scala. Once the changes is okay, I will also apply the changes there.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14219: [SPARK-16566][MLLib] sort sparseVector's indices ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/14219#discussion_r71077904
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/linalg/BLAS.scala ---
@@ -617,8 +617,27 @@ private[spark] object BLAS extends Serializable with Logging {
x: SparseVector,
beta: Double,
y: DenseVector): Unit = {
- val xValues = x.values
- val xIndices = x.indices
+
+ def isSorted(l:Array[Int]): Boolean = {
--- End diff --
Many small style issues here. I think this is best as
```
private def isSorted(array: Array[Int]): Boolean = {
var index = 1
while (index < array.length) {
if (array(index - 1) > array(index)) {
return false
}
index += 1
}
true
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org