xtensor-stack / xtensor

C++ tensors with broadcasting and lazy computing
BSD 3-Clause "New" or "Revised" License
3.31k stars 392 forks source link

Assigning Nx1 view of `pytensor` to `xtensor` using XSIMD gives incorrect results #1092

Closed burnpanck closed 6 years ago

burnpanck commented 6 years ago

This is a continuation of the discussion in issue #389, but likely this is an independent problem. The following C++ code

xt::xtensor<double,2> test(const xt::pytensor<double,2> &a,int idx){
    xt::xtensor<double,2> b;
    switch(idx){
        case 1: b = xt::view(a,xt::all(),xt::range(0,1)); break;
        case 2: b.assign(xt::view(a,xt::all(),xt::range(0,1))); break;
        case 3: b = xt::view(a,xt::all(),xt::range(0,2)); break;
        case 4: b = 2.*xt::view(a,xt::all(),xt::range(0,1)); break;
        case 5: b = xt::view(2.*a,xt::all(),xt::range(0,1)); break;
        default: b=a; break;
    }
    return b;
}
PYBIND11_MODULE(_cpp, m) {
    xt::import_numpy();
    m.def("test",test);
}

with this python code

a = np.array([
    9, 6, 3,
    8, 5, 2,
    7, 4, 1,
]).reshape(3,3)
for k in range(6):
    b = _cpp.test(a,k)
    print(k)
    print(b)

outputs

0
[[ 9.  6.  3.]
 [ 8.  5.  2.]
 [ 7.  4.  1.]]
1
[[ 9.]
 [ 9.]
 [ 9.]]
2
[[ 9.]
 [ 9.]
 [ 9.]]
3
[[ 9.  6.]
 [ 8.  5.]
 [ 7.  4.]]
4
[[ 18.]
 [ 18.]
 [ 18.]]
5
[[ 18.]
 [ 16.]
 [ 14.]]

Examples 0, 3 and 5 are correct, the others incorrect. Particularly, only direct Nx1 views of pytensor fail, but not expressions involving them, nor larger views. The setup is xtensor 0.17.0 (0.17.1 or 2 or whatever the most current is failed compilation somewhere else deep inside our larger project), xtensor-python 0.20.0 and xsimd 6.1.4. If I recompile without -DXTENSOR_USE_XSIMD, all examples return correct results.

wolfv commented 6 years ago

Thanks! Looking into it.

wolfv commented 6 years ago

can confirm that this happens with xtensor, too. investigating.

wolfv commented 6 years ago

Hey, I've prepared a fix for this issue here https://github.com/QuantStack/xtensor/pull/1095

If all tests pass, we'll merge + make a release tonight! Thanks for finding this.

wolfv commented 6 years ago

btw i hope it's okay that i reused your code in the test.

burnpanck commented 6 years ago

Sure