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

Bug when adding vector to a matrix #773

Closed aliakhtar closed 4 years ago

aliakhtar commented 4 years ago

I'm encountering a bug when adding a vector to a matrix as described in the docs. The following works:

val dm = DenseMatrix((1.0,2.0), (3.0,4.0))
dm(::, *) + DenseVector(1.0, 1.0) 
res2: breeze.linalg.DenseMatrix[Double] =
2.0  3.0
4.0  5.0

But if I add a 3rd row to the matrix, and try to add the same vector to it, I get an error:

val dm = DenseMatrix((1.0,2.0), (3.0,4.0), (5.0, 6.0))
dm(::, *) + DenseVector(1.0, 1.0) 
java.lang.IllegalArgumentException: requirement failed: Vectors must have same length: x.length == y.length (2 != 3)
  at breeze.linalg.DenseVector$canDaxpy$.apply(DenseVector.scala:694)
  at breeze.linalg.DenseVector$$anon$13.apply(DenseVector.scala:684)
  at breeze.linalg.DenseVector$$anon$13.apply(DenseVector.scala:682)
  at breeze.linalg.operators.DenseVector_GenericOps$$anon$312.apply(DenseVectorOps.scala:774)
  at breeze.linalg.operators.DenseVector_GenericOps$$anon$312.apply(DenseVectorOps.scala:771)
  at breeze.linalg.BroadcastedColumns$$anon$4.$anonfun$apply$3(BroadcastedColumns.scala:95)
  at breeze.linalg.BroadcastedColumns$$anon$4$$Lambda$5532.000000005CDEAB90.apply(Unknown Source)
  at breeze.linalg.DenseMatrix$$anon$19.$anonfun$apply$2(DenseMatrix.scala:881)
  at breeze.linalg.DenseMatrix$$anon$19.$anonfun$apply$2$adapted(DenseMatrix.scala:880)
  at breeze.linalg.DenseMatrix$$anon$19$$Lambda$5533.000000005CDEB9B0.apply(Unknown Source)
  at scala.collection.immutable.Range.foreach(Range.scala:190)
  at breeze.linalg.DenseMatrix$$anon$19.apply(DenseMatrix.scala:880)
  at breeze.linalg.DenseMatrix$$anon$19.apply(DenseMatrix.scala:877)
  at breeze.linalg.BroadcastedColumns$$anon$4.apply(BroadcastedColumns.scala:95)
  at breeze.linalg.BroadcastedColumns$$anon$4.apply(BroadcastedColumns.scala:93)
  at breeze.linalg.NumericOps.$plus(NumericOps.scala:153)
  at breeze.linalg.NumericOps.$plus$(NumericOps.scala:152)
  at breeze.linalg.BroadcastedColumns.$plus(BroadcastedColumns.scala:30)
  ... 38 elided

It looks like this issue happens if the number of rows in the matrix go higher than the length of the vector, i.e with 2 rows and a 2D vector, it works, but with 3 rows, it fails.

Any ideas why this is? Its got to be a bug, right? I'd be happy to try to send a pull to fix this, if given some pointers.

aliakhtar commented 4 years ago

Stackoverflow indicates this wasn't an issue in the past: https://stackoverflow.com/questions/46025694/add-a-vector-to-every-column-of-a-matrix-using-scala-breeze

Looks like this assert should just be removed: https://github.com/scalanlp/breeze/blob/8c216beb11cce13ea97559835ee1c65fcd3df2bc/math/src/main/scala/breeze/linalg/DenseVector.scala#L708

bluesheeptoken commented 4 years ago

Hey,

I do not know if this is still up to date, but I think the assert is at the right place.

dm in your second example has 3 rows and 2 columns:

DenseMatrix((1.0,2.0), (3.0,4.0), (5.0, 6.0))
2.0  3.0
4.0  5.0
6.0  7.0

So adding a 2 rows vector should result in a fail.

However, you can add a 3 row vector : dm(::, *) + DenseVector(1.0, 1.0, 1.0), or broadcast through rows: dm(*, ::) + DenseVector(1.0, 1.0)