gonum / matrix

Matrix packages for the Go language [DEPRECATED]
446 stars 53 forks source link

matrix/mat64: question: re-use of memory for mat64.Vector pointers seems confusing #427

Closed ChristopherRabotin closed 7 years ago

ChristopherRabotin commented 7 years ago

In the following code, it seems that vec1 is not re-initialized at each loop. Is that correct, and if so, is that the intended behavior? I was incredibly confused at this behavior for several hours last night while trying to debug a piece of code.

The following tests were ran with go version go1.7.1 linux/amd64.

Re-use of memory example

In the following, since vec1 is created inside the loop, I would expect each new iteration to start from scratch and overwrite the vec1 pointer entirely. However, it behaves as if vec1 was defined once before the start of the loop.

Code

func main() {
    values := []float64{1, 2, 3}
    for i := 0; i < 3; i++ {
        vec1 := mat64.NewVector(3, values)
        vec2 := mat64.NewVector(3, []float64{4, 5, 6})
        vec1.SubVec(vec1, vec2)
        fmt.Printf("%+v\n", vec1)
    }
}

Output

&{mat:{Inc:1 Data:[-3 -3 -3]} n:3}
&{mat:{Inc:1 Data:[-7 -8 -9]} n:3}
&{mat:{Inc:1 Data:[-11 -13 -15]} n:3

Fix for previous situation

func main() {
    for i := 0; i < 3; i++ {
        values := []float64{1, 2, 3}
        vec1 := mat64.NewVector(3, values)
        vec2 := mat64.NewVector(3, []float64{4, 5, 6})
        vec1.SubVec(vec1, vec2)
        fmt.Printf("%+v\n", vec1)
    }
}

Output

&{mat:{Inc:1 Data:[-3 -3 -3]} n:3}
&{mat:{Inc:1 Data:[-3 -3 -3]} n:3}
&{mat:{Inc:1 Data:[-3 -3 -3]} n:3}
btracey commented 7 years ago

It is the intended behavior. NewVector (as well as NewDense, and the like), uses the actual slice provided as memory. That is, it does not allocate and copy.

What could be done to avoid confusion? It's a use case we need to support for a variety of reasons. It's also clearly documented; from NewVector: If len(data) == n, data is used as the backing data slice.

kortschak commented 7 years ago

The Go equivalent is https://play.golang.org/p/M4l99eafAV

package main

import (
    "fmt"
)

func main() {
    values := [...]float64{1, 2, 3}
    for i := 0; i < 3; i++ {
        vec1 := values[:]
        vec2 := []float64{4, 5, 6}
        subVecInto(vec1, vec1, vec2)
        fmt.Printf("%+v\n", vec1)
    }
}

func subVecInto(dst, a, b []float64) {
    if len(dst) != len(a) || len(dst) != len(b) {
        panic("length mismatch")
    }
    for i := range a {
        dst[i] = a[i] - b[i]
    }
}

I think this is a commonly used and well documented approach within Go idiom. Maybe the documentation could be more explicit. Though I see that NewDense is actually less clear on this than NewVector, NewSymDense and NewTriDense which all explicitly state that data/mat will be used (maybe changing mat -> data would be good too.

btracey commented 7 years ago

Yes. I wonder if there's some value in having a NewVectorFrom([]float64) where the data is copied? It is true that NewXxx goes against our normal convention of data is copied.

kortschak commented 7 years ago

No sorry, I meant the docs differ. I think the behaviour is correct here.

btracey commented 7 years ago

Sorry, I agree the behavior is correct, but I was wondering if there's value in having a second function which copies the data instead.

kortschak commented 7 years ago

No, I don't think so. Maybe there is something we could do in the docs or in the wiki documentation we had been planning.

ChristopherRabotin commented 7 years ago

NewVector's doc has "data is used as the backing data slice". I guess that I didn't interpret this correctly when I read that a few months back. Maybe changing "backing data slide" to "data storage" or just adding "(i.e. operations will rewrite the initial slice)".

kortschak commented 7 years ago

I think taking wording from the slice blog post and slice internals blog post would be the best. The semantics are identical as seen above.