search

v923z / micropython-ulab

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

Fix repr a #534

Closed jepler closed 2 years ago

dpgeorge commented 2 years ago

Sorry I broke this. A better fix would be to wrap the constants in MICROPY_FLOAT_CONST().

v923z commented 2 years ago

Sorry I broke this. A better fix would be to wrap the constants in MICROPY_FLOAT_CONST().

But is it not in the definition? https://github.com/v923z/micropython-ulab/blob/1347694672dc76ae2d6d74fd2c76cccab7dfc311/code/ndarray.h#L49-L50

v923z commented 2 years ago

@jepler Thanks for catching this! I am wondering, whether the CI should also test the various representations. It might not be worthwhile, though.

@dpgeorge Is there any other place, where the various representations could behave differently, except for the definitions of the constants?

dpgeorge commented 2 years ago

Actually you're right, MICROPY_FLOAT_CONST() is already used.

So what exactly was broken? This fix seems a bit strange..

I am wondering, whether the CI should also test the various representations.

Might be a good idea to build with 32-bit float, I think that's what was broken here. Does CI already build with single precision?

v923z commented 2 years ago

Actually you're right, MICROPY_FLOAT_CONST() is already used.

So what exactly was broken? This fix seems a bit strange..

This is a question for @jepler. However, I noticed that https://github.com/v923z/micropython-builder also failed to produce the artifacts, with the sole exception of the PYBD_SF6 platform. I just haven't had time to find out what went astray.

I am wondering, whether the CI should also test the various representations.

Might be a good idea to build with 32-bit float, I think that's what was broken here. Does CI already build with single precision?

Nope. If we add the 32-bit floats, does it make sense to run it for the full matrix (all four dimensions, unix, mac), or do you think we can get away with a single build? The default 2 dimensions has the most extensive test suite, so I would just go with that.

dpgeorge commented 2 years ago

If we add the 32-bit floats, does it make sense to run it for the full matrix (all four dimensions, unix, mac), or do you think we can get away with a single build?

I don't think it needs a full matrix. Might be good to build all four dimensions, but only need to do it on unix.

jepler commented 2 years ago

Pasting 'INFINITY' with 'F' via the MICROPY_FLOAT_CONST macro:

../../extmod/ulab/code/numpy/numpy.c:64:60: error: pasting ")" and "F" does not give a valid preprocessing token

Pasting '2.7...f' (from MP_E) with 'F' (double application of MICROPY_FLOAT_CONST?) via the MICROPY_FLOAT_CONST macro:

../../extmod/ulab/code/numpy/../ndarray.h:27:34: error: invalid suffix "FF" on floating constant

I think it's entirely possible there are better solutions to this, I did it in a quick but cowardly way.

dpgeorge commented 2 years ago

See #535 for an alternative fix to this PR.