Skip to content

[query] Add method to go from BlockMatrix to NDArray#10398

Merged
danking merged 7 commits intohail-is:mainfrom
johnc1231:johnc-expose-bm-collect
Apr 29, 2021
Merged

[query] Add method to go from BlockMatrix to NDArray#10398
danking merged 7 commits intohail-is:mainfrom
johnc1231:johnc-expose-bm-collect

Conversation

@johnc1231
Copy link
Contributor

I mostly want this for debugging lowering on LocalBackend, but I added a SparkBackend implementation as well (goes through breeze and java literals, not going to be super performant).

@johnc1231
Copy link
Contributor Author

cc @pwc2 , might be of interest to you, though I think you're more interested in going from NDArrays to BlockMatrices.

@johnc1231
Copy link
Contributor Author

The tests are going to fail the local backend for now because they use methods like from_numpy which aren't lowered yet.

val breezeMat = bm.toBreezeMatrix()
val shape = IndexedSeq(bm.nRows, bm.nCols)
// transpose because breeze toArray is column major
SafeNDArray(shape, breezeMat.t.toArray)
Copy link
Member

Choose a reason for hiding this comment

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

If there's no way to avoid an actual transpose, it would be better to do it before collecting to breeze, so you're only transposing the blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In breeze transpose is just a isTransposed flag, so this should just be telling it which way to collect.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but then the collect is the transpose. You're collecting a column major matrix in row major order, which for larger matrices will have terrible cache behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the change anyway.

@danking danking merged commit 558509b into hail-is:main Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants