v923z / micropython-ulab

a numpy-like fast vector module for micropython, circuitpython, and their derivatives
https://micropython-ulab.readthedocs.io/en/latest
MIT License
430 stars 117 forks source link

vectorize gives incorrect results #564

Closed v923z closed 1 year ago

v923z commented 1 year ago

Simply taking the bug report from https://github.com/adafruit/circuitpython/issues/7376

jepler commented 1 year ago

@v923z I can verify there's something odd going on. I built current ulab for micropython with the built.sh script then ran this simplified reproducer:

from ulab import numpy as np

new_poses = np.array([
    [1, 2, 3],
    [4, 5, 6],
    [7, 8, 9],
], dtype=np.float)

identity = np.vectorize(lambda n: n)
intermediate = new_poses[:,2]
output = identity(intermediate)
output2 = np.array([n for n in intermediate])
print("With vectorize:", repr(output.tolist()))
print("With list comprehension:", repr(output2.tolist()))

It prints:

With vectorize: [3.0, 4.0, 5.0]
With list comprehension: [3.0, 6.0, 9.0]
jepler commented 1 year ago

Thank for taking a look at it!

v923z commented 1 year ago

@dannystaple I can't really compile and test in the next two days, but I'm pretty sure I know where the problem is, so if this is urgent, then here is a fix: you would have to replace https://github.com/v923z/micropython-ulab/blob/1a440d7d124a20a3dbaa0909000741eb28a623b3/code/numpy/vector.c#L762-L766 by the following code:

        uint8_t *sarray = (uint8_t *)source->array;
        uint8_t *narray = (uint8_t *)ndarray->array;

        for(size_t u = 0; u < source->len; u++) {
            avalue[0] = mp_binary_get_val_array(source->dtype, sarray, 0);
            fvalue = MP_OBJ_TYPE_GET_SLOT(self->type, call)(self->fun, 1, 0, avalue);
            ndarray_set_value(self->otypes, narray, 0, fvalue);
            narray += ndarray->itemsize;
            // move source pointer to new location
            #if ULAB_MAX_DIMS > 3
            size_t i = 0;
            do {
            #endif
                #if ULAB_MAX_DIMS > 2
                size_t j = 0;
                do {
                #endif
                    #if ULAB_MAX_DIMS > 1
                    size_t k = 0;
                    do {
                    #endif
                        size_t l = 0;
                        do {
                            sarray += source->strides[ULAB_MAX_DIMS - 1];
                            l++;
                        } while(l < source->shape[ULAB_MAX_DIMS - 1]);
                    #if ULAB_MAX_DIMS > 1
                        sarray -= source->strides[ULAB_MAX_DIMS - 1] * source->shape[ULAB_MAX_DIMS - 1];
                        sarray += source->strides[ULAB_MAX_DIMS - 2];
                        k++;
                    } while(k < source->shape[ULAB_MAX_DIMS - 2]);
                    #endif /* ULAB_MAX_DIMS > 1 */
                #if ULAB_MAX_DIMS > 2
                    sarray -= source->strides[ULAB_MAX_DIMS - 2] * source->shape[ULAB_MAX_DIMS - 2];
                    sarray += source->strides[ULAB_MAX_DIMS - 3];
                    j++;
                } while(j < source->shape[ULAB_MAX_DIMS - 3]);
                #endif /* ULAB_MAX_DIMS > 2 */
            #if ULAB_MAX_DIMS > 3
                sarray -= source->strides[ULAB_MAX_DIMS - 3] * source->shape[ULAB_MAX_DIMS - 3];
                sarray += source->strides[ULAB_MAX_DIMS - 4];
                i++;
            } while(i < source->shape[ULAB_MAX_DIMS - 4]);
            #endif /* ULAB_MAX_DIMS > 3 */
        }

If you add this, and that works, you could create a PR with the modifications.

Alternatively, since the original code works for dense arrays, you could simply create a deep copy of your slice:

intermediate = new_poses[:,2].copy()

At that point, intermediate becomes a dense array, so iterations would no longer be problematic.

v923z commented 1 year ago

https://github.com/v923z/micropython-ulab/pull/568 fixes the issue, closing now.