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 Vector subset is assigned to itself? #765

Closed bellaz89 closed 4 years ago

bellaz89 commented 4 years ago

When a subsect of a vector is assigned to itself it gives an incorrect result. Example

val a = DenseVector(1,2,3,4)
a(1 to 3) := a(0 to 2)
// returns  a = DenseVector(1, 1, 1, 1)
// I would have expected a = DenseVector(1, 1, 2, 3) instead.

It seems that the code is not managing the sanity of copy operations when the destination memory is equal to the origin memory. In other libraries (eg. numpy) it works as intended. Probably the same happens for matrices...

bellaz89 commented 4 years ago

I think there are 2 possible solutions for this (if it is a bug at all..):

  1. A la python. Every time an in-place operation happens a copy of the memory of the passed object is created. This will have some impact on performance. Some mitigations on the performace hit of this might be possible if the method makes a copy of the origin vector only if it detects that the origin memory reference is equal to the dest. one (using eq on the storage memory?).

  2. A la Eigen https://eigen.tuxfamily.org/dox/group__TopicAliasing.html . Use a.copy . It is then the user's responsibility to use such a method when he performs an in-place operation and dest = orig.. It can complicate to produce correct programs when Vector and Matrices references are sliced and passed multiple times..

In the latter case, I would in any case strongly recommend making it really clear that the sanity of in-place operations is the user's responsibility modifying the Doc (Quickstart.. Linalg). This is a really important point given that breeze is often compared to numpy in the docs. Then, to not discourage the newcomers the differences between the libraries should be very well underlined.

dlwh commented 4 years ago

Yeah this is a case I missed. I thought I fixed these. I'll try to get to it soon

dlwh commented 4 years ago

(It appears I stopped receiving email notifications. Sorry about that!)

dlwh commented 4 years ago

this ended up being hilariously complicated to do "right". it's now a bit too conservative, but only a bit.