v923z / micropython-ulab

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

[BUG] crash when numpy.concatenate() given scalars #572

Closed dhalbert closed 1 year ago

dhalbert commented 1 year ago

ulab version is commit e68bb707 (current CircuitPython main version). ulab.__version__ reports 6.0.2-2D, but I think that commit is past that?

To Reproduce

>>> import ulab
>>> ulab.numpy.concatenate((2.0,4.0,6.0), axis=1)
[crash]
Auto-reload is off.
Running in safe mode! Not running saved code.

You are in safe mode because:
CircuitPython core code crashed hard. Whoops!
Crash into the HardFault_Handler.

Expected behavior I believe the above is incorrect because concatenate() is being passed a sequence of scalars instead of a sequence of arrays. (This was some user code). But it should not crash. CPython numpy reports an error:

>>> numpy.concatenate((2.0,4.0,6.0), axis=1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<__array_function__ internals>", line 5, in concatenate
ValueError: zero-dimensional arrays cannot be concatenated

This is not a showstopper bug, since it involves incorrect usage.

v923z commented 1 year ago

ulab version is commit e68bb70 (current CircuitPython main version). ulab.__version__ reports 6.0.2-2D, but I think that commit is past that?

I don't know what you mean. The version number was bumped in that commit: https://github.com/v923z/micropython-ulab/commit/e68bb707b20ee326d84ab75fc9fb35f2e85b87e3

numpy.concatenate((2.0,4.0,6.0), axis=1) Traceback (most recent call last): File "", line 1, in File "<__array_function__ internals>", line 5, in concatenate ValueError: zero-dimensional arrays cannot be concatenated

This is not a showstopper bug, since it involves incorrect usage.

It should still be fixed, thanks for bringing it up!

dhalbert commented 1 year ago

I don't know what you mean. The version number was bumped in that commit: e68bb70

Ah, I didn't know that. I was looking at the ulab GitHub releases, and thought that the commit was past any of those, so I assumed there was not a version bump yet.

Aside: The bug issue template says:

A clear and concise description of what the bug is. Give the ulab version

from ulab import numpy as np
print(np.__version__)

but np.__version__ was not set. I had to do ulab.__version__.

v923z commented 1 year ago

I don't know what you mean. The version number was bumped in that commit: e68bb70

Ah, I didn't know that. I was looking at the ulab GitHub releases, and thought that the commit was past any of those, so I assumed there was not a version bump yet.

Oh, that! I don't usually create a new release, if it is just a bug fix. So, in that sense the releases are not up-to-date.

Aside: The bug issue template says:

A clear and concise description of what the bug is. Give the ulab version

from ulab import numpy as np
print(np.__version__)

but np.__version__ was not set. I had to do ulab.__version__.

That's right. If you think it is necessary to have a version number for the sub-modules, I'm not against the idea, but on the other hand, it could be a bit odd, if sub-modules started to diverge.

You can, on the other hand, include the commit hash in the firmware: https://github.com/v923z/micropython-ulab/blob/e68bb707b20ee326d84ab75fc9fb35f2e85b87e3/code/ulab.c#L220-L222 You just have to define the constant somewhere, or pass it from the command line.

dhalbert commented 1 year ago

I meant that if click on "New Issue", and choose "Bug", the bug template says to fetch the version from np.__version__, instead of saying to use ulab.__version__. But np.__version__ doesn't work, the latter does. So I think the issue template needs to be corrected: https://github.com/v923z/micropython-ulab/blob/master/.github/ISSUE_TEMPLATE/bug_report.md

v923z commented 1 year ago

I meant that if click on "New Issue", and choose "Bug", the bug template says to fetch the version from np.__version__, instead of saying to use ulab.__version__. But np.__version__ doesn't work, the latter does. So I think the issue template needs to be corrected: https://github.com/v923z/micropython-ulab/blob/master/.github/ISSUE_TEMPLATE/bug_report.md

OK, now I get it. Thanks for bringing this up! I've corrected the template.

v923z commented 1 year ago

https://github.com/v923z/micropython-ulab/pull/575 fixes the issue. numpy allows array-likes in general (concatenate), but here I would really like to keep this to ndarrays only, given that array-likes would give an overhead not only in terms of code, but RAM, too. This overhead would not be obvious to the user, which I would find disturbing on a small MCU. If one wants to concatenate lists, or tuples, they can still be explicitly cast like

np.concatenate((np.array([1, 2]), np.array([3, 4]))

I hope this is a satisfactory solution.