[GitHub] [spark] zhengruifeng commented on a change in pull request #27519: [SPARK-30770][ML][WIP] avoid vector conversion in GMM.transform

2020-02-25 Thread GitBox 
zhengruifeng commented on a change in pull request #27519: 
[SPARK-30770][ML][WIP] avoid vector conversion in GMM.transform
URL: https://github.com/apache/spark/pull/27519#discussion_r384252384
 
 

 ##
 File path: 
mllib-local/src/main/scala/org/apache/spark/ml/stat/distribution/MultivariateGaussian.scala
 ##
 @@ -48,43 +48,37 @@ class MultivariateGaussian @Since("2.0.0") (
 this(Vectors.fromBreeze(mean), Matrices.fromBreeze(cov))
   }
 
-  @transient private lazy val breezeMu = mean.asBreeze.toDenseVector
-
   /**
* Compute distribution dependent constants:
*rootSigmaInv = D^(-1/2)^ * U.t, where sigma = U * D * U.t
*u = log((2*pi)^(-k/2)^ * det(sigma)^(-1/2)^)
*/
-  @transient private lazy val tuple = calculateCovarianceConstants
-  @transient private lazy val rootSigmaInv = tuple._1
+  @transient private lazy val tuple = {
+val (rootSigmaInv, u) = calculateCovarianceConstants
+val rootSigmaInvMat = Matrices.fromBreeze(rootSigmaInv)
+(rootSigmaInvMat, u)
+  }
+  @transient private lazy val rootSigmaInvMat = tuple._1
   @transient private lazy val u = tuple._2
+  @transient private lazy val mu = mean.toDense
 
   /**
* Returns density of this multivariate Gaussian at given point, x
*/
   @Since("2.0.0")
   def pdf(x: Vector): Double = {
-pdf(x.asBreeze)
+math.exp(logpdf(x))
 
 Review comment:
   Yes, that is what existing impl does:
   ```scala
 /**
  * Returns density of this multivariate Gaussian at given point, x
  */
 @Since("2.0.0")
 def pdf(x: Vector): Double = {
   pdf(x.asBreeze)
 }
   
 /**
  * Returns the log-density of this multivariate Gaussian at given point, x
  */
 @Since("2.0.0")
 def logpdf(x: Vector): Double = {
   logpdf(x.asBreeze)
 }
   
 /** Returns density of this multivariate Gaussian at given point, x */
 private[ml] def pdf(x: BV[Double]): Double = {
   math.exp(logpdf(x))
 }
   
 /** Returns the log-density of this multivariate Gaussian at given point, 
x */
 private[ml] def logpdf(x: BV[Double]): Double = {
   val delta = x - breezeMu
   val v = rootSigmaInv * delta
   u + v.t * v * -0.5
 }
   
   ```
   
   After this change, we do not need private methods based on BreezeVector.


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


With regards,
Apache Git Services

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] [spark] zhengruifeng commented on a change in pull request #27519: [SPARK-30770][ML][WIP] avoid vector conversion in GMM.transform

2020-02-17 Thread GitBox 
zhengruifeng commented on a change in pull request #27519: 
[SPARK-30770][ML][WIP] avoid vector conversion in GMM.transform
URL: https://github.com/apache/spark/pull/27519#discussion_r380030567
 
 

 ##
 File path: 
mllib-local/src/main/scala/org/apache/spark/ml/stat/distribution/MultivariateGaussian.scala
 ##
 @@ -48,43 +48,40 @@ class MultivariateGaussian @Since("2.0.0") (
 this(Vectors.fromBreeze(mean), Matrices.fromBreeze(cov))
   }
 
-  @transient private lazy val breezeMu = mean.asBreeze.toDenseVector
-
   /**
* Compute distribution dependent constants:
*rootSigmaInv = D^(-1/2)^ * U.t, where sigma = U * D * U.t
*u = log((2*pi)^(-k/2)^ * det(sigma)^(-1/2)^)
*/
-  @transient private lazy val tuple = calculateCovarianceConstants
-  @transient private lazy val rootSigmaInv = tuple._1
-  @transient private lazy val u = tuple._2
+  @transient private lazy val tuple3 = {
+val (rootSigmaInv, u) = calculateCovarianceConstants
+val rootSigmaInvMat = Matrices.fromBreeze(rootSigmaInv)
+val rootSigmaInvMulMu = rootSigmaInvMat.multiply(mean)
+(rootSigmaInvMat, u, rootSigmaInvMulMu)
+  }
+
+  @transient private lazy val rootSigmaInvMat = tuple3._1
+
+  @transient private lazy val u = tuple3._2
+
+  @transient private lazy val rootSigmaInvMulMu = tuple3._3
 
   /**
* Returns density of this multivariate Gaussian at given point, x
*/
   @Since("2.0.0")
   def pdf(x: Vector): Double = {
-pdf(x.asBreeze)
+math.exp(logpdf(x))
   }
 
   /**
* Returns the log-density of this multivariate Gaussian at given point, x
*/
   @Since("2.0.0")
   def logpdf(x: Vector): Double = {
-logpdf(x.asBreeze)
-  }
-
-  /** Returns density of this multivariate Gaussian at given point, x */
-  private[ml] def pdf(x: BV[Double]): Double = {
-math.exp(logpdf(x))
-  }
-
-  /** Returns the log-density of this multivariate Gaussian at given point, x 
*/
-  private[ml] def logpdf(x: BV[Double]): Double = {
-val delta = x - breezeMu
-val v = rootSigmaInv * delta
-u + v.t * v * -0.5
+val v = rootSigmaInvMulMu.copy
+BLAS.gemv(-1.0, rootSigmaInvMat, x, 1.0, v)
+u - 0.5 * BLAS.dot(v, v)
 
 Review comment:
   `rootSigmaInv * delta = rootSigmaInv * (x - breezeMu) = - (rootSigmaInv * 
breezeMu - rootSigmaInv * x)`


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


With regards,
Apache Git Services

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] [spark] zhengruifeng commented on a change in pull request #27519: [SPARK-30770][ML][WIP] avoid vector conversion in GMM.transform

2020-02-16 Thread GitBox 
zhengruifeng commented on a change in pull request #27519: 
[SPARK-30770][ML][WIP] avoid vector conversion in GMM.transform
URL: https://github.com/apache/spark/pull/27519#discussion_r376937067
 
 

 ##
 File path: 
mllib-local/src/main/scala/org/apache/spark/ml/stat/distribution/MultivariateGaussian.scala
 ##
 @@ -43,46 +43,40 @@ class MultivariateGaussian @Since("2.0.0") (
   require(cov.numCols == cov.numRows, "Covariance matrix must be square")
   require(mean.size == cov.numCols, "Mean vector length must match covariance 
matrix size")
 
-  /** Private constructor taking Breeze types */
-  private[ml] def this(mean: BDV[Double], cov: BDM[Double]) = {
-this(Vectors.fromBreeze(mean), Matrices.fromBreeze(cov))
-  }
-
-  @transient private lazy val breezeMu = mean.asBreeze.toDenseVector
-
   /**
* Compute distribution dependent constants:
*rootSigmaInv = D^(-1/2)^ * U.t, where sigma = U * D * U.t
*u = log((2*pi)^(-k/2)^ * det(sigma)^(-1/2)^)
*/
-  @transient private lazy val (rootSigmaInv: BDM[Double], u: Double) = 
calculateCovarianceConstants
+  @transient private lazy val tuple3 = {
 
 Review comment:
   it is said in 
[LeastSquaresAggregator](https://github.com/apache/spark/blob/12e1bbaddbb2ef304b5880a62df6683fcc94ea54/mllib/src/main/scala/org/apache/spark/ml/optim/aggregator/LeastSquaresAggregator.scala#L188)
 that 
   
   > // do not use tuple assignment above because it will circumvent the 
@transient tag


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


With regards,
Apache Git Services

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org