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
359 stars 49 forks source link

tensor.Mul bug #70

Closed wzzhu closed 4 years ago

wzzhu commented 4 years ago

The tenor.Mul has bug in multiplying tensor with shape(1, 1). See the test code below. This might be caused by https://github.com/gorgonia/tensor/pull/54

The reason is that e.E.Mul(typ, dataA, retVal.hdr()) always assuming the result would be kept in dataA.

func TestMul(t *testing.T) {
    a := 2.0
    b := tensor.NewDense(tensor.Float64, tensor.Shape{1, 1}, tensor.WithBacking([]float64{3}))
    var correct interface{} = []float64{6.0}

    res, err := tensor.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)
}
bezineb5 commented 4 years ago

The problem occurs when multiplying a true scalar with a scalar tensor, correct? For example, if you put "a" in a tensor, does it works as expected?

wzzhu commented 4 years ago

The problem occurs when multiplying a true scalar with a scalar tensor, correct? For example, if you put "a" in a tensor, does it works as expected?

It still fails if changing "a" as

a := tensor.NewDense(tensor.Float64, tensor.Shape{1}, tensor.WithBacking([]float64{2}))

This bug is serious since it affects gorgonia's solvers (Affecting AdamSolver, VanillaSolver and others, but not RMSPropSolver!) when using

if _, err = tensor.Mul(eta, g, tensor.UseUnsafe()); err != nil {

where eta is a {}interface {float value}

Note that RMSPropSolver uses "tensor.Mul(g, eta, ...) and avoids the problem.

bezineb5 commented 4 years ago

I made a fix, I hope that will solve your problems.