[SPARK-12869] Implemented an improved version of the toIndexedRowMatrix

[Fokko]

Hi guys,

I've implemented an improved version of the toIndexedRowMatrix function on the BlockMatrix. I needed this for a project, but would like to share it with the rest of the community. In the case of dense matrices, it can increase performance up to 19 times:
https://github.com/Fokko/BlockMatrixToIndexedRowMatrix

If there are any questions or suggestions, please let me know. Keep up the good work! Cheers.

[hvanhovell]

cc @mengxr

[MLnick]

ok to test

[MLnick]

ok to test

[SparkQA]

Test build #52080 has finished for PR 10839 at commit 4d7c297.

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

[SparkQA]

Test build #53036 has finished for PR 10839 at commit 67fd902.

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

[mengxr]

This assumes that a partition can hold an entire block row, which is not always valid. I would suggest the following:

• for each block, break the matrix block into rows and then emit (rowIdx, (colStartIndex, row)). You can map the matrix block to a breeze matrix, and then call rows.
• call groupByKey and then concat breeze vectors. Note that there could be missing vectors.

This could be a more scalable implementation.

[Fokko]

Thanks @mengxr, that's a very good idea. I'll update the code and push it within 24 hours. Cheers!

[Fokko]

I've improved the PR based on the feedback. Beside that I've also updated the benchmark:
https://github.com/Fokko/BlockMatrixToIndexedRowMatrix

If there are any questions, please let me know.

[SparkQA]

Test build #53239 has finished for PR 10839 at commit a9bc894.

• This patch fails to build.
• This patch does not merge cleanly.
• This patch adds no public classes.

[Fokko]

ok to test

[SparkQA]

Test build #53238 has finished for PR 10839 at commit ba7791f.

• This patch passes all tests.
• This patch does not merge cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #53240 has finished for PR 10839 at commit fe1842e.

• This patch passes all tests.
• This patch does not merge cleanly.
• This patch adds no public classes.

[mengxr]

@Fokko The implementation doesn't take care of sparsity yet. I created #11757 to add row/column iterators to local matrices. After that one gets merged, you can simply the implementation here.

[Fokko]

Nice work, as soon as the PR will be merged I will update the code accordingly.

[mengxr]

@Fokko That PR was merged. Could you merge the current master and update your implementation? Note that when you concat the vectors, it is useful to check the sparsity and then decide whether to create a dense vector or a sparse vector. Allocating a dense vector directly could be expensive.

[SparkQA]

Test build #53401 has finished for PR 10839 at commit c043e77.

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

[SparkQA]

Test build #53419 has finished for PR 10839 at commit d3c780d.

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

[Fokko]

@mengxr I've updated the code according to your PR :)

[mengxr]

The output vector type should depend on the total number of active elements (or nonzeros) instead of the first one. Could you try vectors.map(_.activeSize).sum and compare it with numCols to decide which vector type to use?

[mengxr]

Btw, you also need to merge with the current master to resolve conflicts.

[SparkQA]

Test build #53722 has finished for PR 10839 at commit 25c5f66.

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

[Fokko]

@mengxr did you have a chance to look at the updated version? I also extended the test to check the conversion to dense/sparse vectors.

[mengxr]

Sorry for late response! This LGTM. Merged into master. Thanks!