[SPARK-11826][MLlib] Refactor add() and subtract() methods #9916
Conversation
|
Test build #46551 has finished for PR 9916 at commit
|
|
Test build #46644 has finished for PR 9916 at commit
|
|
I'd defer to @mengxr on this one for final approval but yes that's definitely what I had in mind. It's nice that you refactored add and subtract together |
ehsanmok
changed the title
|
@mengxr Could you please check this? |
|
@mengxr @srowen @jkbradley Why reviewing this simple thing which is important for my application is taking so long? |
|
@ehsanmok Apologies for the slow review. We do constantly have ~100 pending PRs and many more JIRAs, so they can be hard to cover with limited reviewer bandwidth. I'll take a look now. |
|
@jkbradley thank you! I'm guess that'd be suitable for Spark 1.6.1 so Since annotations should be updated, right? |
| * if it is being added to a [[DenseMatrix]]. If two dense matrices are added, the output will | ||
| * also be a [[DenseMatrix]]. | ||
| * For given matrices `this` and `other` of compatible dimensions and compatible block dimensions, | ||
| * it applies an associative binary function on their corresponding blocks. |
There was a problem hiding this comment.
not associative (subtraction is not)
|
Test build #49436 has finished for PR 9916 at commit
|
|
Test build #49439 has finished for PR 9916 at commit
|
|
@jkbradley Thank you for your instructive and detailed feedbacks. It should be fine now. Please review it. |
| .map { case ((blockRowIndex, blockColIndex), (a, b)) => | ||
| if (a.size > 1 || b.size > 1) { | ||
| throw new SparkException("There are multiple MatrixBlocks with indices: " + | ||
| s"($blockRowIndex, $blockColIndex). Please remove them.") | ||
| } | ||
| if (a.isEmpty) { | ||
| new MatrixBlock((blockRowIndex, blockColIndex), b.head) | ||
| val zeroBlock = BM.zeros[Double](b.head.numRows, b.head.numCols) |
There was a problem hiding this comment.
This is making an explicit matrix of all 0s. It would be better to create a sparse matrix of all zeros to avoid the memory allocation.
There was a problem hiding this comment.
I would have said use CSCMatrix.zeros(rows, cols). What do you mean by "then would have to add every zero for later operations?"
There was a problem hiding this comment.
The problem now is that; there's no implicits for adding and subtracting CSCMatrix and BM! So should I leave that as is then?
There was a problem hiding this comment.
@jkbradley so I'll modify the doc (as mentioned below) and commit. I don't think there's a better option available for zero sparse matrices!
|
Test build #49893 has finished for PR 9916 at commit
|
|
@jkbradley could you please check the update soon? |
|
@ehsanmok Sorry for the delay. Could you please add a to-do in the code for using zero matrices more efficiently? Other than that, it looks fine. Would you be interested in submitting a PR to Breeze to add a primitive for subtracting a DenseMatrix and CSCMatrix? If you can, please CC me on the PR, and I can help review it. Thanks! |
|
@jkbradley I'd really like to but don't have enough time these days. I'll add that to my to-do list and will let you know then. |
|
Test build #52263 has finished for PR 9916 at commit
|
|
@jkbradley I hope it doesn't take another month waiting to get merged! |
|
Yes, I was checking about whether it'd be OK to merge with this less efficient implementation. I think so, though, so I'll go ahead and merge it after re-running fresh tests. |
|
Test build #2623 has finished for PR 9916 at commit
|
|
Test build #2640 has finished for PR 9916 at commit
|
|
Merging with master |
srowen Could you please check this when you have time? Author: Ehsan M.Kermani <ehsanmo1367@gmail.com> Closes apache#9916 from ehsanmok/JIRA-11826.
@srowen Could you please check this when you have time?