search

scalanlp / breeze

Breeze is/was a numerical processing library for Scala.
https://www.scalanlp.org
Apache License 2.0
3.45k stars 693 forks source link

Add method to multiply dense vectors with CSC matrices #809

Open aaronquantexa opened 3 years ago

aaronquantexa commented 3 years ago

Hi, I saw this issue https://github.com/scalanlp/breeze/issues/715 and thought I'd have a go seeing as it's a good first issue (over 1 year old!).

I've copied the style of code around the repo and lightly cloned this test: "SparseVector * CSCMatrix Lifted OpMulMatrix & Transpose" to validate the matrix multiplication is correct for DenseVectors.

To allow the multiplication I've extracted 2 conversions (DenseVector to CSCMatrix and CSCMatrix to DenseVector - this latter one has an assertion inside that the Matrix has just 1 row), which are public.

Do let me know if there is more testing / other changes required.

dlwh commented 3 years ago

Hi @aaronquantexa, thanks for the PR!

Two things:

1) I think it makes more sense to get there via conversion from the DenseMatrix/CSCMatrix implementation from here https://github.com/scalanlp/breeze/blob/a1c56461b6b364eb4644a4ec1c5cdbc93b9bddec/math/src/main/codegen/breeze/linalg/operators/CSCMatrixOps.scala#L679 rather than the CSC/CSC, since a Transpose[DenseVector] is conceptually just a DenseMatrix with 1 row.

2) I'm in the middle of very large and obnoxious refactor to get Scala 3 support working, which has necessitated moving around a whole lot of stuff. If this doesn't merge cleanly into that branch, I may just copy and paste from your PR rather than merging, with due credit of course.

aaronquantexa commented 3 years ago

Cool, I'll have a go at using DenseMatrix.

On point 2, sounds fair - I'll leave this to you as maintainer.

aaronquantexa commented 3 years ago

If it doesn't merge easily, can remove the additional - now redundant functions - in DenseVector.scala

jczestochowska commented 3 years ago

related question: is subtraction between CSCMatrix and DenseVector supported? I tried - operator and all other related - operators but it seems like it's not implemented. @aaronquantexa @dlwh