Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-7681][MLlib] Add SparseVector support for gemv #6209

Closed
wants to merge 9 commits into from

Conversation

viirya
Copy link
Member

@viirya viirya commented May 16, 2015

@SparkQA
Copy link

SparkQA commented May 16, 2015

Test build #32885 has finished for PR 6209 at commit c069507.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@viirya
Copy link
Member Author

viirya commented May 16, 2015

retest this please.

@SparkQA
Copy link

SparkQA commented May 16, 2015

Test build #32889 has finished for PR 6209 at commit c069507.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

case (dense: DenseMatrix, dx: DenseVector) =>
gemv(alpha, dense, dx, beta, y)
case (dense: DenseMatrix, sx: SparseVector) =>
gemv(alpha, dense, sx, beta, y)
case _ =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about SparseMatrix and SparseVector? To make the consistent naming, we can use dmA, smA, dvx, and svx.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't really want to add SparseMatrix and SparseVector, the type safety will be broken when you call with this configuration. Previously, this function is totally type safe in compile time, and no way to get into "case _".

@viirya viirya changed the title [SPARK-7681][MLlib] Add SparseVector support for gemv with DenseMatrix [SPARK-7681][MLlib] Add SparseVector support for gemv May 17, 2015
@SparkQA
Copy link

SparkQA commented May 17, 2015

Test build #32936 has finished for PR 6209 at commit 4616696.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

var xNnz = xIndices.size
var xValues = x.values

scal(beta, y)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we should check it?

@dbtsai
Copy link
Member

dbtsai commented May 17, 2015

Finally in sealed trait Matrix, change

  /** Convenience method for `Matrix`-`DenseVector` multiplication. */
  def multiply(y: DenseVector): DenseVector = {

to

  /** Convenience method for `Matrix`-`Vector` multiplication. */
  def multiply(y: Vector): DenseVector = {

@debasish83
Copy link

Are there runtime comparisons posted with vector*vector operations for these changes BLAS-1 vs BLAS-2 ? SparseMatrix * SparseVector compared to Array[SparseVector] x SparseVector

var i = Acols(colCounterForA)
val indEnd = Acols(colCounterForA + 1)

val xVal = xValues(k) * alpha
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xVal will be easily to read as xValues. simply make it as xTemp

val xVal = xValues(k) * alpha
while (i < indEnd) {
val rowIndex = Arows(i)
yValues(rowIndex) += Avals(i) * xVal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yValues(Arows(i)) += Avals(i) * xTemp

@SparkQA
Copy link

SparkQA commented May 18, 2015

Test build #32981 has finished for PR 6209 at commit 410381a.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented May 18, 2015

Test build #32982 has finished for PR 6209 at commit 054f05d.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@viirya
Copy link
Member Author

viirya commented May 18, 2015

retest this please.

@SparkQA
Copy link

SparkQA commented May 18, 2015

Test build #32986 has finished for PR 6209 at commit 054f05d.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented May 18, 2015

Test build #32988 has finished for PR 6209 at commit 458d1ae.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented May 18, 2015

Test build #32989 has finished for PR 6209 at commit 57a8c1e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dbtsai
Copy link
Member

dbtsai commented May 18, 2015

LGTM cc: @mengxr @srowen @jkbradley

@mengxr
Copy link
Contributor

mengxr commented May 18, 2015

I will take a pass on the changes.

/** Convenience method for `Matrix`-`DenseVector` multiplication. */
def multiply(y: DenseVector): DenseVector = {
/** Convenience method for `Matrix`-`Vector` multiplication. */
def multiply(y: Vector): DenseVector = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot delete def multiply(y: DenseVector), which breaks binary compatibility. Please delegate the implementation to multiply(y: Vector) and update Mima excludes.

@viirya
Copy link
Member Author

viirya commented May 19, 2015

I just found the original gemv for SparseMatrix and DenseVector has a bug that it only scales y when beta != 0.0. We should fix it too.

@SparkQA
Copy link

SparkQA commented May 19, 2015

Test build #33040 has finished for PR 6209 at commit b890e63.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • public class JavaOneVsRestExample
    • head("OneVsRest Example: multiclass to binary reduction using OneVsRest")
    • class DataTypeSingleton(type):
    • class NullType(DataType):
    • class AtomicType(DataType):
    • class NumericType(AtomicType):
    • class IntegralType(NumericType):
    • class FractionalType(NumericType):
    • class StringType(AtomicType):
    • class BinaryType(AtomicType):
    • class BooleanType(AtomicType):
    • class DateType(AtomicType):
    • class TimestampType(AtomicType):
    • class DecimalType(FractionalType):
    • class DoubleType(FractionalType):
    • class FloatType(FractionalType):
    • class ByteType(IntegralType):
    • class IntegerType(IntegralType):
    • class LongType(IntegralType):
    • class ShortType(IntegralType):
    • class Column(object):
    • class GroupedData(object):

@SparkQA
Copy link

SparkQA commented May 19, 2015

Test build #33042 has finished for PR 6209 at commit ce0bb8b.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

asfgit pushed a commit that referenced this pull request May 19, 2015
JIRA: https://issues.apache.org/jira/browse/SPARK-7681

Author: Liang-Chi Hsieh <viirya@gmail.com>

Closes #6209 from viirya/sparsevector_gemv and squashes the following commits:

ce0bb8b [Liang-Chi Hsieh] Still need to scal y when beta is 0.0 because it clears out y.
b890e63 [Liang-Chi Hsieh] Do not delete multiply for DenseVector.
57a8c1e [Liang-Chi Hsieh] Add MimaExcludes for v1.4.
458d1ae [Liang-Chi Hsieh] List DenseMatrix.multiply and SparseMatrix.multiply to MimaExcludes too.
054f05d [Liang-Chi Hsieh] Fix scala style.
410381a [Liang-Chi Hsieh] Address comments. Make Matrix.multiply more generalized.
4616696 [Liang-Chi Hsieh] Add support for SparseVector with SparseMatrix.
5d6d07a [Liang-Chi Hsieh] Merge remote-tracking branch 'upstream/master' into sparsevector_gemv
c069507 [Liang-Chi Hsieh] Add SparseVector support for gemv with DenseMatrix.

(cherry picked from commit d03638c)
Signed-off-by: Xiangrui Meng <meng@databricks.com>
@asfgit asfgit closed this in d03638c May 19, 2015
@mengxr
Copy link
Contributor

mengxr commented May 19, 2015

LGTM. Merged into master and branch-1.4. I will test the Mima excludes to make them minimal. Please update the naive Bayes PR. Thanks!

jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request May 28, 2015
JIRA: https://issues.apache.org/jira/browse/SPARK-7681

Author: Liang-Chi Hsieh <viirya@gmail.com>

Closes apache#6209 from viirya/sparsevector_gemv and squashes the following commits:

ce0bb8b [Liang-Chi Hsieh] Still need to scal y when beta is 0.0 because it clears out y.
b890e63 [Liang-Chi Hsieh] Do not delete multiply for DenseVector.
57a8c1e [Liang-Chi Hsieh] Add MimaExcludes for v1.4.
458d1ae [Liang-Chi Hsieh] List DenseMatrix.multiply and SparseMatrix.multiply to MimaExcludes too.
054f05d [Liang-Chi Hsieh] Fix scala style.
410381a [Liang-Chi Hsieh] Address comments. Make Matrix.multiply more generalized.
4616696 [Liang-Chi Hsieh] Add support for SparseVector with SparseMatrix.
5d6d07a [Liang-Chi Hsieh] Merge remote-tracking branch 'upstream/master' into sparsevector_gemv
c069507 [Liang-Chi Hsieh] Add SparseVector support for gemv with DenseMatrix.
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request Jun 12, 2015
JIRA: https://issues.apache.org/jira/browse/SPARK-7681

Author: Liang-Chi Hsieh <viirya@gmail.com>

Closes apache#6209 from viirya/sparsevector_gemv and squashes the following commits:

ce0bb8b [Liang-Chi Hsieh] Still need to scal y when beta is 0.0 because it clears out y.
b890e63 [Liang-Chi Hsieh] Do not delete multiply for DenseVector.
57a8c1e [Liang-Chi Hsieh] Add MimaExcludes for v1.4.
458d1ae [Liang-Chi Hsieh] List DenseMatrix.multiply and SparseMatrix.multiply to MimaExcludes too.
054f05d [Liang-Chi Hsieh] Fix scala style.
410381a [Liang-Chi Hsieh] Address comments. Make Matrix.multiply more generalized.
4616696 [Liang-Chi Hsieh] Add support for SparseVector with SparseMatrix.
5d6d07a [Liang-Chi Hsieh] Merge remote-tracking branch 'upstream/master' into sparsevector_gemv
c069507 [Liang-Chi Hsieh] Add SparseVector support for gemv with DenseMatrix.
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
JIRA: https://issues.apache.org/jira/browse/SPARK-7681

Author: Liang-Chi Hsieh <viirya@gmail.com>

Closes apache#6209 from viirya/sparsevector_gemv and squashes the following commits:

ce0bb8b [Liang-Chi Hsieh] Still need to scal y when beta is 0.0 because it clears out y.
b890e63 [Liang-Chi Hsieh] Do not delete multiply for DenseVector.
57a8c1e [Liang-Chi Hsieh] Add MimaExcludes for v1.4.
458d1ae [Liang-Chi Hsieh] List DenseMatrix.multiply and SparseMatrix.multiply to MimaExcludes too.
054f05d [Liang-Chi Hsieh] Fix scala style.
410381a [Liang-Chi Hsieh] Address comments. Make Matrix.multiply more generalized.
4616696 [Liang-Chi Hsieh] Add support for SparseVector with SparseMatrix.
5d6d07a [Liang-Chi Hsieh] Merge remote-tracking branch 'upstream/master' into sparsevector_gemv
c069507 [Liang-Chi Hsieh] Add SparseVector support for gemv with DenseMatrix.
@viirya viirya deleted the sparsevector_gemv branch December 27, 2023 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants