zhengruifeng commented on a change in pull request #31394:
URL: https://github.com/apache/spark/pull/31394#discussion_r569996166
##########
File path:
mllib/src/main/scala/org/apache/spark/ml/feature/BucketedRandomProjectionLSH.scala
##########
@@ -97,7 +97,19 @@ class BucketedRandomProjectionLSHModel private[ml](
@Since("2.1.0")
override protected[ml] def hashDistance(x: Seq[Vector], y: Seq[Vector]):
Double = {
// Since it's generated by hashing, it will be a pair of dense vectors.
- x.zip(y).map(vectorPair => Vectors.sqdist(vectorPair._1,
vectorPair._2)).min
+ // Currently each hash vector (generated by hashFunction) only has one
element, this equals to:
Review comment:
> So with N hash functions you get N 1-vectors, not 1 N-vector?
Yes, for both MinHash and BucketedRandomProjectionLSH.
> is this assumption always going to be true for these two implementations,
so we don't need to assert about it?
It seems that community had try to update this to N M-vectors, but seems
inactive for a long time.
> to accommodate future algorithms that need to return longer vectors
It maybe possible, so I tend to update this PR to not use the attribute of
1-Vector.
> stop wrapping all these in vectors to begin with
I think we can not do this, since this column of type `Array[Vector]` had
been already exposed to end user.
LSH is widely used, but current impl of LSH in mllib does not work well in
my opinion. I will study it in the future.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
