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

MaskedTensor has some fundamental arithmetic issues if we follow Numpy's conventions. #6

Open chewxy opened 7 years ago

chewxy commented 7 years ago

From @chewxy on July 9, 2017 22:30

func TestMaskedPlay(t *testing.T) {
    a := New(WithShape(2, 2), WithBacking([]float64{1, 2, 3, 4}), WithMask([]bool{true, false, false, false}))
    t.Logf("a \n%v", a)
    t.Logf("%v | %v", a.mask, a.l)

    b := New(WithShape(2, 2), WithBacking([]float64{1, 2, 3, 4}))
    t.Logf("b \n%v", b)

    incr := New(WithShape(2, 2), WithBacking([]float64{100, 100, 100, 100}))
    StdEng{}.Add(a, b, WithIncr(incr))
    t.Logf("\n%v ", incr)
}

Results in:

    masked_test.go:7: a 
        ⎡--   2⎤
        ⎣ 3   4⎦
    masked_test.go:8: [true false false false] | 4
    masked_test.go:11: b 
        ⎡1  2⎤
        ⎣3  4⎦
    masked_test.go:21: 
        ⎡100  104⎤
        ⎣106  108⎦

By and large this makes sense. However, current tests fail mainly because the original APIs were built to be mirrors of Numpy's API for masked arrays. In Numpy:

>>> import numpy as np
>>> import numpy.ma as ma
>>> y = ma.array([1, 2, 3], mask = [0, 1, 0])

>>> incr = np.array([100,100,100])
>>> x = np.array([2,4,6])
>>> incr += x + y
>>> incr
array([103, 104, 109])
>>> x
array([2, 4, 6])
>>> y
masked_array(data = [1 -- 3],
             mask = [False  True False],
       fill_value = 999999)

>>> incr = np.array([100,100,100])
>>> incr += x * y
>>> incr
array([102, 104, 118])

The last answer is pretty WTF-ing. It violates an arithmetic convention (PEDMAS)

@kabaka0 care to weigh in? What should the correct way be?

Copied from original issue: chewxy/gorgonia#133

chewxy commented 7 years ago

Relevant issue and discussion: https://github.com/numpy/numpy/issues/9394

chewxy commented 7 years ago

@kabaka0 any input on the behaviours of masked tensors?

chewxy commented 7 years ago

Additionally, maskedDense seems to not transpose correctly (I'll actually write proper tests to see if that's the case)