srowen commented on a change in pull request #31394:
URL: https://github.com/apache/spark/pull/31394#discussion_r568753035
##########
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:
Huh, that seems odd. So with N hash functions you get N 1-vectors, not 1
N-vector? I read SPARK-18454 referred to in the comments but wasn't clear why
it was done this way.
Since you may have thought about this more - is this assumption always going
to be true for these two implementations, so we don't need to assert about it?
or do we need to check the dim to make sure this doesn't return the wrong
answer if it ever changes?
Like was this put in place, do you think, to accommodate future algorithms
that need to return longer vectors?
If not I wonder if this is another optimization opportunity, to stop
wrapping all these in vectors to begin with.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
