gorgonia / tensor

package tensor provides efficient and generic n-dimensional arrays in Go that are useful for machine learning and deep learning purposes
Apache License 2.0
362 stars 49 forks source link

MulScalar bug #52

Closed chewxy closed 4 years ago

chewxy commented 5 years ago

@bezineb5 discovered this and opened #51

The bug is described as follows:

a2 := New(WithBacking([]float64{2}))
    b2 := New(WithBacking([]float64{3}))
    var correct interface{} = 6.0

    res, err := Mul(a2, b2)
    if err != nil {
        t.Fatalf("Error: %v", err)
    }
    assert.Equal(t, correct, res.Data())
    t.Logf("a2 %v b2 %v, res %v", a2, b2, res)
    a := New(WithBacking([]float64{3, 2}))
    b := New(WithBacking([]float64{2}))
    correct := []float64{6, 4}

    res, err := Mul(a, b)
    if err != nil {
        t.Fatalf("Error: %v", err)
    }
    assert.Equal(t, correct, res.Data())
    t.Logf("a %v b %v, res %v", a, b, res)

    // Test commutativity
    res, err = Mul(b, a)
    if err != nil {
        t.Fatalf("Error: %v", err)
    }
    assert.Equal(t, correct, res.Data())
    t.Logf("a %v b %v, res %v", a, b, res)

The bug happens in https://github.com/gorgonia/tensor/blob/master/defaultengine_arith.go#L665 and https://github.com/gorgonia/tensor/blob/master/defaultengine_arith.go#L639 as well.

The bug affects all generated SV and VS arithmetic functions.

chewxy commented 5 years ago

Source of generated code https://github.com/gorgonia/tensor/blob/master/genlib2/agg2_body.go#L39

bezineb5 commented 5 years ago

So, I had a look at this issue, and fixed this in defaultengine_arith, line 665:

        if leftTensor {
            retVal = a.Clone().(Tensor)
        } else {
            retVal = s.(Tensor).Clone().(Tensor)
        }
        err = e.E.Mul(typ, retVal.hdr(), dataB)

The only thing is that e.E.Mul checks which one of its parameters is scalar and modifies the other one - and modifies the first in case both are scalar. So the output value has to be the leftmost value.

How do I run the genlib2 to regenerate the code?

chewxy commented 4 years ago

Fixed so closing

wzzhu commented 4 years ago

Not fixed correctly yet See https://github.com/gorgonia/tensor/issues/70