[WIP] Fix ndarray constructor

[CallumJHays]

WIP fix for #301

• Still missing constructor args other than 'shape'
• Not yet implemented for circuitpython

Hey @v923z could you give this a look and let me know if there's a better way to write this?

[v923z]

@CallumJHays Cal, I am struggling to understand, what this PR is trying to fix: while

a = array(5)

is perfectly valid in numpy, this seems to create a strange sort of array. E.g., I can't even read out the value of the element:

a[0]

---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)
<ipython-input-81-544b26bc25e9> in <module>
      1 a = array(5)
----> 2 a[0]

IndexError: too many indices for array: array is 0-dimensional, but 1 were indexed

And while the initialisation doesn't raise an exception, this mode is not mentioned in the official documentation: https://numpy.org/doc/stable/reference/generated/numpy.array.html#numpy.array. The first argument should be something that looks like an array, walks like an array, and quacks like an array.

I am a bit flabbergasted to say the least. Not by the PR, but by the fact that there seems to be a bit of an inconsistency in numpy itself.

[CallumJHays]

@v923z The main point of the PR will only affect ndarray.__init__(), following the numpy behaviour:

>>> import numpy as np
>>> np.ndarray([1, 2, 3])
array([[[4.63760270e-310, 0.00000000e+000, 4.63760245e-310],
        [6.91622088e-310, 1.92343089e-313, 6.91625378e-310]]])
>>> np.array([1, 2, 3])
array([1, 2, 3])

I believe currently both np.ndarray([1, 2, 3]) and np.array([1, 2, 3]) result in the same output.

As for accepting an int rather than either an int or iterable this was just the behaviour I noticed when testing numpy locally so I replicated it. I'm not well-versed on the nuances of numpy's API, but I would have to assume it could have benefits when using scalar arithmetic in numpy with the data already being in C memory. I can definitely see it having benefits in streamlining some usage of the numpy API in a pythonic duck-typing way. I also agree the numpy documentation is inconsistent.

Something I wanted to bring up btw is supporting kwargs that match the position of a positional arg, ie let this work:

>>> np.array(object=[1, 2, 3])
array([1, 2, 3])
>>> np.ndarray(shape=[1, 2, 3])
array([[[4.63760270e-310, 0.00000000e+000, 4.63760245e-310],
        [6.91622088e-310, 1.92343089e-313, 6.91625378e-310]]])

Last time I checked it threw an error similar to "required positional argument not included". Is there a way we can fix this without firmware bloat?

[v923z]

@v923z The main point of the PR will only affect ndarray.__init__(), following the numpy behaviour:

>>> import numpy as np
>>> np.ndarray([1, 2, 3])
array([[[4.63760270e-310, 0.00000000e+000, 4.63760245e-310],
        [6.91622088e-310, 1.92343089e-313, 6.91625378e-310]]])
>>> np.array([1, 2, 3])
array([1, 2, 3])

I believe currently both np.ndarray([1, 2, 3]) and np.array([1, 2, 3]) result in the same output.

Thanks for the explanation! If you move the code block to make_new_core, I can merge the PR.

As for accepting an int rather than either an int or iterable this was just the behaviour I noticed when testing numpy locally so I replicated it. I'm not well-versed on the nuances of numpy's API, but I would have to assume it could have benefits when using scalar arithmetic in numpy with the data already being in C memory. I can definitely see it having benefits in streamlining some usage of the numpy API in a pythonic duck-typing way. I also agree the numpy documentation is inconsistent.

Something I wanted to bring up btw is supporting kwargs that match the position of a positional arg, ie let this work:

>>> np.array(object=[1, 2, 3])
array([1, 2, 3])
>>> np.ndarray(shape=[1, 2, 3])
array([[[4.63760270e-310, 0.00000000e+000, 4.63760245e-310],
        [6.91622088e-310, 1.92343089e-313, 6.91625378e-310]]])

Last time I checked it threw an error similar to "required positional argument not included". Is there a way we can fix this without firmware bloat?

I am not against this option, if you deem this to be useful. The keyword argument will add approx. 200-400 bytes to the firmware size. If you find that to be too much, you could make that configurable via a ULAB_NDARRAY_ACCEPTS_KEYWORDS or something similar pre-processor switch.

[CallumJHays]

@v923z that last commit has the basic functionality of passing shape in, and the tests have been corrected. Sorry for dragging my feet but work and social life has been hectic this past week.

Do you think we should handle strides as well? I'm not sure if an alternative already exists in ulab other than setting the property after instantiation.

Also, do you know of a standard way to populate a C arr from a micropython iterator?, like a:

size_t len = 3;
uint8_t arr[len];
MP_ITER_FILL_ARR(mp_iter, uint8_t,  arr, len);

[v923z]

@v923z that last commit has the basic functionality of passing shape in, and the tests have been corrected. Sorry for dragging my feet but work and social life has been hectic this past week.

Oh, don't worry, this is a project run by enthusiasts. I am not going to hold you to any specific deadline, and I am grateful for any contribution. By the way, thanks for patching up the doc stubs!

Do you think we should handle strides as well? I'm not sure if an alternative already exists in ulab other than setting the property after instantiation.

If you think it is useful, I don't see any reason not to. But I don't see pressing reasons for, either. I leave it up to you.

Also, do you know of a standard way to populate a C arr from a micropython iterator?, like a:

size_t len = 3;
uint8_t arr[len];
MP_ITER_FILL_ARR(mp_iter, uint8_t,  arr, len);

Do you mean something like ndarray_assign_elements? https://github.com/v923z/micropython-ulab/blob/96a944cc38fb04e5636a5b62a3c048b9c3de2b9e/code/ndarray.c#L567 This is not a macro, but you get the gist.

[v923z]

@CallumJHays do you want to go ahead with this PR? I could take over, if you have no time. I would just like to close the issue, because I think this would be a valuable addition.