Skip to content
Closed
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
b693209
Ready for Pull request
Nov 11, 2014
f378e16
[SPARK-3974] Block Matrix Abstractions ready
Nov 11, 2014
aa8f086
[SPARK-3974] Additional comments added
Nov 11, 2014
589fbb6
[SPARK-3974] Code review feedback addressed
Nov 14, 2014
19c17e8
[SPARK-3974] Changed blockIdRow and blockIdCol
Nov 14, 2014
b05aabb
[SPARK-3974] Updated tests to reflect changes
brkyvz Nov 14, 2014
645afbe
[SPARK-3974] Pull latest master
brkyvz Nov 14, 2014
49b9586
[SPARK-3974] Updated testing utils from master
brkyvz Nov 14, 2014
d033861
[SPARK-3974] Removed SubMatrixInfo and added constructor without part…
brkyvz Nov 15, 2014
9ae85aa
[SPARK-3974] Made partitioner a variable inside BlockMatrix instead o…
brkyvz Nov 20, 2014
ab6cde0
[SPARK-3974] Modifications cleaning code up, making size calculation …
brkyvz Jan 14, 2015
ba414d2
[SPARK-3974] fixed frobenius norm
brkyvz Jan 14, 2015
239ab4b
[SPARK-3974] Addressed @jkbradley's comments
brkyvz Jan 19, 2015
1e8bb2a
[SPARK-3974] Change return type of cache and persist
brkyvz Jan 20, 2015
1a63b20
[SPARK-3974] Remove setPartition method. Isn't required
brkyvz Jan 20, 2015
eebbdf7
preliminary changes addressing code review
brkyvz Jan 21, 2015
f9d664b
updated API and modified partitioning scheme
brkyvz Jan 21, 2015
1694c9e
almost finished addressing comments
brkyvz Jan 27, 2015
140f20e
Merge branch 'master' of github.com:apache/spark into SPARK-3974
brkyvz Jan 27, 2015
5eecd48
fixed gridPartitioner and added tests
brkyvz Jan 27, 2015
24ec7b8
update grid partitioner
mengxr Jan 28, 2015
e1d3ee8
minor updates
mengxr Jan 28, 2015
feb32a7
update tests
mengxr Jan 28, 2015
a8eace2
Merge pull request #2 from mengxr/brkyvz-SPARK-3974
brkyvz Jan 28, 2015
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,242 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.spark.mllib.linalg.distributed

import breeze.linalg.{DenseMatrix => BDM}

import org.apache.spark.{Logging, Partitioner}
import org.apache.spark.mllib.linalg._
import org.apache.spark.mllib.rdd.RDDFunctions._
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is treeAggregate in getDim

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I didn't see mllib ...

import org.apache.spark.rdd.RDD
import org.apache.spark.storage.StorageLevel

/**
* A grid partitioner, which stores every block in a separate partition.
*
* @param numRowBlocks Number of blocks that form the rows of the matrix.
* @param numColBlocks Number of blocks that form the columns of the matrix.
*/
private[mllib] class GridPartitioner(
val numRowBlocks: Int,
val numColBlocks: Int,
val numParts: Int) extends Partitioner {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add doc.

Remove val. Otherwise, numParts is a public field of GridPartitioner. We may also rename it to suggestedNumPartitions so users know that it might change.

// Having the number of partitions greater than the number of sub matrices does not help
override val numPartitions = math.min(numParts, numRowBlocks * numColBlocks)

/**
* Returns the index of the partition the SubMatrix belongs to. Tries to achieve block wise
* partitioning.
*
* @param key The key for the SubMatrix. Can be its position in the grid (its column major index)
* or a tuple of three integers that are the final row index after the multiplication,
* the index of the block to multiply with, and the final column index after the
* multiplication.
* @return The index of the partition, which the SubMatrix belongs to.
*/
override def getPartition(key: Any): Int = {
key match {
case (blockRowIndex: Int, blockColIndex: Int) =>
getBlockId(blockRowIndex, blockColIndex)
case (blockRowIndex: Int, innerIndex: Int, blockColIndex: Int) =>
getBlockId(blockRowIndex, blockColIndex)
case _ =>
throw new IllegalArgumentException(s"Unrecognized key. key: $key")
}
}

/** Partitions sub-matrices as blocks with neighboring sub-matrices. */
private def getBlockId(blockRowIndex: Int, blockColIndex: Int): Int = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be called getPartition or getPartitionId?

val totalBlocks = numRowBlocks * numColBlocks
// Gives the number of blocks that need to be in each partition
val partitionRatio = math.ceil(totalBlocks * 1.0 / numPartitions).toInt
Copy link
Contributor

Choose a reason for hiding this comment

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

partitionRatio -> targetNumBlocksPerPartition?

// Number of neighboring blocks to take in each row
val subBlocksPerRow = math.ceil(numRowBlocks * 1.0 / partitionRatio).toInt
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong. If we have 10x20 blocks and 10 partitions. partitionRatio = 20 and subBlocksPerRow = 1 and subBlocksPerCol = 1. Then we will have 200 partitions. Please update the assignment login and add tests. Btw, it may be worth computing subBlocksPerRow and subBlocksPerCol in constructor. I would recommend renaming them to numRowBlocksPerPartition and numColBlocksPerPartition.

// Number of neighboring blocks to take in each column
val subBlocksPerCol = math.ceil(numColBlocks * 1.0 / partitionRatio).toInt
// Coordinates of the block
val i = blockRowIndex / subBlocksPerRow
val j = blockColIndex / subBlocksPerCol
val blocksPerRow = math.ceil(numRowBlocks * 1.0 / subBlocksPerRow).toInt
j * blocksPerRow + i
}

/** Checks whether the partitioners have the same characteristics */
override def equals(obj: Any): Boolean = {
obj match {
case r: GridPartitioner =>
(this.numRowBlocks == r.numRowBlocks) && (this.numColBlocks == r.numColBlocks) &&
(this.numPartitions == r.numPartitions)
case _ =>
false
}
}
}

/**
* Represents a distributed matrix in blocks of local matrices.
*
* @param rdd The RDD of SubMatrices (local matrices) that form this matrix
* @param nRows Number of rows of this matrix
Copy link
Contributor

Choose a reason for hiding this comment

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

Document the behavior when nRows is negative, e.g., whether this is allowed. I'm okay with marking them as required.

* @param nCols Number of columns of this matrix
* @param numRowBlocks Number of blocks that form the rows of this matrix
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be derived from nRows and rowsPerBlock? Having nRows, numRowBlocks, and rowsPerBlock would leave space for inconsistent inputs.

* @param numColBlocks Number of blocks that form the columns of this matrix
* @param rowsPerBlock Number of rows that make up each block. The blocks forming the final
* rows are not required to have the given number of rows
* @param colsPerBlock Number of columns that make up each block. The blocks forming the final
* columns are not required to have the given number of columns
*/
class BlockMatrix(
val rdd: RDD[((Int, Int), Matrix)],
private var nRows: Long,
private var nCols: Long,
val numRowBlocks: Int,
val numColBlocks: Int,
val rowsPerBlock: Int,
val colsPerBlock: Int) extends DistributedMatrix with Logging {

private type SubMatrix = ((Int, Int), Matrix) // ((blockRowIndex, blockColIndex), matrix)

/**
* Alternate constructor for BlockMatrix without the input of the number of rows and columns.
*
* @param rdd The RDD of SubMatrices (local matrices) that form this matrix
* @param numRowBlocks Number of blocks that form the rows of this matrix
* @param numColBlocks Number of blocks that form the columns of this matrix
* @param rowsPerBlock Number of rows that make up each block. The blocks forming the final
* rows are not required to have the given number of rows
* @param colsPerBlock Number of columns that make up each block. The blocks forming the final
* columns are not required to have the given number of columns
*/
def this(
rdd: RDD[((Int, Int), Matrix)],
numRowBlocks: Int,
numColBlocks: Int,
rowsPerBlock: Int,
colsPerBlock: Int) = {
Copy link
Contributor

Choose a reason for hiding this comment

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

The list of arguments cannot provide the complete info about the matrix. For example, if the last block row and the last block column are all missing. Then you cannot figure out the exact matrix size from this list of arguments.

It would be necessary to have numRows, numCols, rowsPerBlock, colsPerBlock, and the RDD as input. We can provide factory methods (in follow-up PRs) to create block matrices from other formats, which could figure out the exact numRows and numCols and use them in the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will it really be the case that the whole row of blocks will be missing for the last row? That means that those rows (or columns) contain no information. Then why store (use) them?

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 make such assumption about the data. It is not rare that we have an empty column/row, which is the last column/row and the only column/row in the last column/row block. For example, in the popular mnist-digit dataset, the last column of the training data is empty.

this(rdd, 0L, 0L, numRowBlocks, numColBlocks, rowsPerBlock, colsPerBlock)
}

private[mllib] var partitioner: GridPartitioner =
new GridPartitioner(numRowBlocks, numColBlocks, rdd.partitions.length)

private lazy val dims: (Long, Long) = getDim

override def numRows(): Long = {
if (nRows <= 0L) nRows = dims._1
nRows
}

override def numCols(): Long = {
if (nCols <= 0L) nCols = dims._2
nCols
}

/** Returns the dimensions of the matrix. */
private def getDim: (Long, Long) = {
case class MatrixMetaData(var rowIndex: Int, var colIndex: Int,
var numRows: Int, var numCols: Int)
// picks the sizes of the matrix with the maximum indices
def pickSizeByGreaterIndex(example: MatrixMetaData, base: MatrixMetaData): MatrixMetaData = {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we modify base in-place, we should put base as the first parameter. This is the convention used in Spark aggregation.

We should simply this code block. Essentially we want to find the largest row index and col index:

val (rows, cols) = rdd.map { case (blockRowIndex, blockColIndex, mat) =>
  (blockRowIndex * rowsPerBlock + mat.numRows, blockColIndex * colsPerBlock + mat.numCols)
}.reduce((x0, x1) => (math.max(x0._1, x1._1), math.max(x0._2, x1._2)))
(math.max(rows, nRows), math.max(cols, nCols))

if (example.rowIndex > base.rowIndex) {
base.rowIndex = example.rowIndex
base.numRows = example.numRows
}
if (example.colIndex > base.colIndex) {
base.colIndex = example.colIndex
base.numCols = example.numCols
}
base
}

// Aggregate will return an error if the rdd is empty
val lastRowCol = rdd.treeAggregate(new MatrixMetaData(0, 0, 0, 0))(
seqOp = (c, v) => (c, v) match { case (base, ((blockXInd, blockYInd), mat)) =>
pickSizeByGreaterIndex(
new MatrixMetaData(blockXInd, blockYInd, mat.numRows, mat.numCols), base)
},
combOp = (c1, c2) => (c1, c2) match {
case (res1, res2) =>
pickSizeByGreaterIndex(res1, res2)
})
// We add the size of the edge matrices, because they can be less than the specified
// rowsPerBlock or colsPerBlock.
(lastRowCol.rowIndex.toLong * rowsPerBlock + lastRowCol.numRows,
lastRowCol.colIndex.toLong * colsPerBlock + lastRowCol.numCols)
}

/** Returns the Frobenius Norm of the matrix */
def normFro(): Double = {
Copy link
Member

Choose a reason for hiding this comment

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

I vote for norm(name: String) with default "Fro"...or maybe something numeric for entry-wise norms. What do other libraries do?

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this function. We can add it back later.

math.sqrt(rdd.map { mat => mat._2 match {
case sparse: SparseMatrix =>
sparse.values.map(x => math.pow(x, 2)).sum
case dense: DenseMatrix =>
dense.values.map(x => math.pow(x, 2)).sum
}
}.reduce(_ + _))
}

/** Cache the underlying RDD. */
def cache(): BlockMatrix = {
rdd.cache()
this
}

/** Set the storage level for the underlying RDD. */
def persist(storageLevel: StorageLevel): BlockMatrix = {
rdd.persist(storageLevel)
this
}

/** Collect the distributed matrix on the driver as a `DenseMatrix`. */
def toLocalMatrix(): Matrix = {
Copy link
Member

Choose a reason for hiding this comment

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

Rename toLocal?

require(numRows() < Int.MaxValue, "The number of rows of this matrix should be less than " +
s"Int.MaxValue. Currently numRows: ${numRows()}")
require(numCols() < Int.MaxValue, "The number of columns of this matrix should be less than " +
s"Int.MaxValue. Currently numCols: ${numCols()}")
val nRows = numRows().toInt
Copy link
Member

Choose a reason for hiding this comment

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

I'd check numRows and numCols here before converting to Int and throw an error if the matrix is too large.

val nCols = numCols().toInt
val mem = nRows * nCols * 8 / 1000000
Copy link
Contributor

Choose a reason for hiding this comment

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

This may overflow. See #4069.

if (mem > 500) logWarning(s"Storing this matrix will require $mem MB of memory!")

val parts = rdd.collect().sortBy(x => (x._1._2, x._1._1))
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need sort.

val values = new Array[Double](nRows * nCols)
parts.foreach { case ((rowIndex, colIndex), block) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

rowBlockIndex and colBlockIndex

val rowOffset = rowIndex * rowsPerBlock
val colOffset = colIndex * colsPerBlock
var j = 0
val mat = block.toArray
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good if we use foreachActive is that PR is merged first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can modify it in the next 3 upcoming PRs (matrix multiplication-addition, conversions, repartition) if it doesn't.

Copy link
Contributor

Choose a reason for hiding this comment

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

foreachActive is merged:)

while (j < block.numCols) {
var i = 0
val indStart = (j + colOffset) * nRows + rowOffset
val matStart = j * block.numRows
while (i < block.numRows) {
values(indStart + i) = mat(matStart + i)
i += 1
}
j += 1
}
}
new DenseMatrix(nRows, nCols, values)
}

/** Collects data and assembles a local dense breeze matrix (for test only). */
private[mllib] def toBreeze(): BDM[Double] = {
Copy link
Member

Choose a reason for hiding this comment

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

If this is just for testing, then I'd make it private[distributed]. If it should fit with other APIs, then it should return a Matrix, not a DenseMatrix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how it is currently for all distributed matrices, and each return a BDM. Maybe we can change all of them later.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. Yeah, I think we should change that later, but later is fine since it's internal.

val localMat = toLocalMatrix()
new BDM[Double](localMat.numRows, localMat.numCols, localMat.toArray)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.spark.mllib.linalg.distributed

import org.scalatest.FunSuite
import breeze.linalg.{DenseMatrix => BDM}

import org.apache.spark.mllib.linalg.{DenseMatrix, Matrices, Matrix}
import org.apache.spark.mllib.util.MLlibTestSparkContext

// Input values for the tests
private object BlockMatrixSuite {
val m = 5
Copy link
Member

Choose a reason for hiding this comment

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

I would put these fixed values in a private BlockMatrixSuite object and then import them inside the class.

val n = 4
val rowPerPart = 2
val colPerPart = 2
val numRowBlocks = 3
val numColBlocks = 2
}

class BlockMatrixSuite extends FunSuite with MLlibTestSparkContext {

val m = BlockMatrixSuite.m
val n = BlockMatrixSuite.n
val rowPerPart = BlockMatrixSuite.rowPerPart
val colPerPart = BlockMatrixSuite.colPerPart
val numRowBlocks = BlockMatrixSuite.numRowBlocks
val numColBlocks = BlockMatrixSuite.numColBlocks
var gridBasedMat: BlockMatrix = _
type SubMatrix = ((Int, Int), Matrix)

override def beforeAll() {
super.beforeAll()

val entries: Seq[SubMatrix] = Seq(
new SubMatrix((0, 0), new DenseMatrix(2, 2, Array(1.0, 0.0, 0.0, 2.0))),
new SubMatrix((0, 1), new DenseMatrix(2, 2, Array(0.0, 1.0, 0.0, 0.0))),
new SubMatrix((1, 0), new DenseMatrix(2, 2, Array(3.0, 0.0, 1.0, 1.0))),
new SubMatrix((1, 1), new DenseMatrix(2, 2, Array(1.0, 2.0, 0.0, 1.0))),
new SubMatrix((2, 1), new DenseMatrix(1, 2, Array(1.0, 5.0))))

gridBasedMat = new BlockMatrix(sc.parallelize(entries, 2), numRowBlocks, numColBlocks,
rowPerPart, colPerPart)
}

test("size and frobenius norm") {
assert(gridBasedMat.numRows() === m)
assert(gridBasedMat.numCols() === n)
assert(gridBasedMat.normFro() === 7.0)
}

test("toBreeze and toLocalMatrix") {
val expected = BDM(
(1.0, 0.0, 0.0, 0.0),
(0.0, 2.0, 1.0, 0.0),
(3.0, 1.0, 1.0, 0.0),
(0.0, 1.0, 2.0, 1.0),
(0.0, 0.0, 1.0, 5.0))

val dense = Matrices.fromBreeze(expected).asInstanceOf[DenseMatrix]
assert(gridBasedMat.toLocalMatrix() === dense)
assert(gridBasedMat.toBreeze() === expected)
}
}