You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by vinodkc <gi...@git.apache.org> on 2015/09/02 09:01:38 UTC
[GitHub] spark pull request: [SPARK-10414][MLlib]Fix hashcode issues in MLL...
GitHub user vinodkc opened a pull request:
https://github.com/apache/spark/pull/8565
[SPARK-10414][MLlib]Fix hashcode issues in MLLib
Added/Updated hashcode methods in 3 classes
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vinodkc/spark fix_ML_hashcode_issue
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/8565.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 #8565
----
commit a4d256b37b0e4c8acdc9d9bca43588cd90cf1bcd
Author: Vinod K C <vi...@huawei.com>
Date: 2015-09-02T07:01:32Z
Added hashcode method
commit 7878c42d568d4c08a64717e66176b0ecf7ee27ab
Author: Vinod K C <vi...@huawei.com>
Date: 2015-09-02T07:09:31Z
Removed blank lines
commit 28737427ff16c2e4d84540361968c9edef3a79dc
Author: Vinod K C <vi...@huawei.com>
Date: 2015-09-02T09:22:58Z
Updated testcase
----
---
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: [SPARK-10414][MLlib]Fix hashcode issues in MLL...
Posted by vinodkc <gi...@git.apache.org>.
Github user vinodkc commented on a diff in the pull request:
https://github.com/apache/spark/pull/8565#discussion_r38507995
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/linalg/Matrices.scala ---
@@ -278,7 +278,8 @@ class DenseMatrix @Since("1.3.0") (
}
override def hashCode: Int = {
- com.google.common.base.Objects.hashCode(numRows : Integer, numCols: Integer, toArray)
+ val state = Seq(numRows, numCols, Arrays.hashCode(values), isTransposed.hashCode)
--- End diff --
I tried with 'toBreeze.hashCode', but hashCode of BM[Double] is not consistent.
---
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: [SPARK-10414][MLlib]Fix hashcode issues in MLL...
Posted by vinodkc <gi...@git.apache.org>.
Github user vinodkc commented on a diff in the pull request:
https://github.com/apache/spark/pull/8565#discussion_r38522537
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/linalg/Matrices.scala ---
@@ -278,7 +278,8 @@ class DenseMatrix @Since("1.3.0") (
}
override def hashCode: Int = {
- com.google.common.base.Objects.hashCode(numRows : Integer, numCols: Integer, toArray)
+ val state = Seq(numRows, numCols, Arrays.hashCode(values), isTransposed.hashCode)
--- End diff --
I've asked on breeze mailing list and waiting for their reply.
Also I checked breeze CSCMatrix '==' method implementation , it seems '==' uses the input data we passed from SparseMatrix and just hashCode is not implemented. I think this will guarantee that my hashCode implementation will be consistent with Breeze '=='.
All MLlib equals hashCode test cases are passing now
---
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: [SPARK-10414][MLlib]Fix hashcode issues in MLL...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/8565#issuecomment-136956647
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 pull request: [SPARK-10414][MLlib]Fix hashcode issues in MLL...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/8565#discussion_r38506418
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/linalg/Matrices.scala ---
@@ -278,7 +278,8 @@ class DenseMatrix @Since("1.3.0") (
}
override def hashCode: Int = {
- com.google.common.base.Objects.hashCode(numRows : Integer, numCols: Integer, toArray)
+ val state = Seq(numRows, numCols, Arrays.hashCode(values), isTransposed.hashCode)
--- End diff --
Is this consistent with `equals`? wouldn't you want to use `toBreeze.hashCode`?
---
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: [SPARK-10414][MLlib]Fix hashcode issues in MLL...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/8565#discussion_r38508550
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/linalg/Matrices.scala ---
@@ -278,7 +278,8 @@ class DenseMatrix @Since("1.3.0") (
}
override def hashCode: Int = {
- com.google.common.base.Objects.hashCode(numRows : Integer, numCols: Integer, toArray)
+ val state = Seq(numRows, numCols, Arrays.hashCode(values), isTransposed.hashCode)
--- End diff --
Consistent with its own `==`, are you sure? I think we're pushing around the problem then, as it's not clear this hash code is consistent with whatever definition of equality it uses. That bears investigation first.
---
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: [SPARK-10414][MLlib]Fix hashcode issues in MLL...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/8565#discussion_r38622953
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/linalg/Matrices.scala ---
@@ -278,7 +278,8 @@ class DenseMatrix @Since("1.3.0") (
}
override def hashCode: Int = {
- com.google.common.base.Objects.hashCode(numRows : Integer, numCols: Integer, toArray)
+ val state = Seq(numRows, numCols, Arrays.hashCode(values), isTransposed.hashCode)
--- End diff --
Good to double-check that. I think we have to write a `hashCode` that is consistent with Breeze's `equals` for now and leave a to-do to change it to Breeze's later. Although it sounds brittle, it seems like equality must be defined in the obvious way -- equal elements everywhere. The hash code can just be a function of the elements since equality must be. It may need to be a function of non-zero elements and their positions, in order, in order to be efficient for sparse matrices.
---
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: [SPARK-10414][MLlib]Fix hashcode issues in MLL...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/8565#discussion_r38515156
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/linalg/Matrices.scala ---
@@ -278,7 +278,8 @@ class DenseMatrix @Since("1.3.0") (
}
override def hashCode: Int = {
- com.google.common.base.Objects.hashCode(numRows : Integer, numCols: Integer, toArray)
+ val state = Seq(numRows, numCols, Arrays.hashCode(values), isTransposed.hashCode)
--- End diff --
Crazy, you might ask on the breeze list if you have time, to make sure we're not missing something, but that seems like a valid test.
We can avoid using either Breeze method in these implementations in order to write a version that we know is consistent and correct. On the other hand, it's likely that `==` is correct, and it's just that hash code isn't implemented. I suppose it should be possible to write a hash code that must be consistent with the plain meaning of equality for matrices, by making it a function of all (non-zero) elements and their positions or something. That is, is your `hashCode` going to be consistent with Breeze `==`? that's the important thing to guarantee.
---
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: [SPARK-10414][MLlib]Fix hashcode issues in MLL...
Posted by vinodkc <gi...@git.apache.org>.
Github user vinodkc closed the pull request at:
https://github.com/apache/spark/pull/8565
---
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: [SPARK-10414][MLlib]Fix hashcode issues in MLL...
Posted by vinodkc <gi...@git.apache.org>.
Github user vinodkc commented on a diff in the pull request:
https://github.com/apache/spark/pull/8565#discussion_r38609166
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/linalg/Matrices.scala ---
@@ -278,7 +278,8 @@ class DenseMatrix @Since("1.3.0") (
}
override def hashCode: Int = {
- com.google.common.base.Objects.hashCode(numRows : Integer, numCols: Integer, toArray)
+ val state = Seq(numRows, numCols, Arrays.hashCode(values), isTransposed.hashCode)
--- End diff --
Got confirmation from Breeze mailing list that it is an issue and suggested to me to raise an issue in Breeze => https://github.com/scalanlp/breeze/issues/440
Can you please suggest whether we need to wait for Breeze issue fix or change equals method of CSCMatrix & DenseMatrix to match my hashCode implementation?
---
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: [SPARK-10414][MLlib]Fix hashcode issues in MLL...
Posted by vinodkc <gi...@git.apache.org>.
Github user vinodkc commented on the pull request:
https://github.com/apache/spark/pull/8565#issuecomment-138167561
Closing this PR, as there is already an existing JIRA & PR on same issue
---
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: [SPARK-10414][MLlib]Fix hashcode issues in MLL...
Posted by vinodkc <gi...@git.apache.org>.
Github user vinodkc commented on a diff in the pull request:
https://github.com/apache/spark/pull/8565#discussion_r38514784
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/linalg/Matrices.scala ---
@@ -278,7 +278,8 @@ class DenseMatrix @Since("1.3.0") (
}
override def hashCode: Int = {
- com.google.common.base.Objects.hashCode(numRows : Integer, numCols: Integer, toArray)
+ val state = Seq(numRows, numCols, Arrays.hashCode(values), isTransposed.hashCode)
--- End diff --
To verify that, I did this test
'val bm1 = new BSM[Double](values, numRows, numCols, colPtrs, rowIndices)
val bm2 = new BSM[Double](values, numRows, numCols, colPtrs, rowIndices)
if(bm1 == bm2){
println("[==] passed")
if( bm1.hashCode == bm2.hashCode){
println(" [hashCode] passed")
}
else {
println(" [hashCode] failed")
}
}
else{
println(" [==] failed")
}'
Got this output
[==] passed
[hashCode] failed
On further investigation, I noticed , this thirdparty class has implemented only 'equals' method, not 'hashCode'
In that case, shall I change 'toBreeze == m.toBreeze' comparison in equals?
---
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