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

Update mp_obj_type_t definitions for latest MicroPython. #549

Closed jimmo closed 2 years ago

jimmo commented 2 years ago

https://github.com/micropython/micropython/pull/8813 changed the way that constant object type structs are defined in order to save ROM.

I have tried to maintain compatibility with CircuitPython, older-MicroPython-revisons. Hopefully CircuitPython will update to match the new style soon and the compatibility code can be removed.

Unfortunately I could not run the tests (from build.sh) as they don't pass for me even before updating the MicroPython version. This appears to be mostly around formatting of float printing (which is also something that has changed in MicroPython recently, but I tested it with v1.19.1 and it wasn't working there either). So that will need to be fixed before this can be merged, unfortunately I don't have the bandwidth to investigate that.

This PR also fixes build.sh to work with the change from https://github.com/micropython/micropython/pull/9012 that changes the output path for the unix port.

jimmo commented 2 years ago

Fixes https://github.com/v923z/micropython-ulab/issues/548

v923z commented 2 years ago

@jimmo Many thanks for patching this up!

As far as I see, the compilation is OK, only the tests fail, and even then, only in two cases. You are right, there is a glitch with the formatting of small floating point numbers. As a temporary solution, we could take those out of the test suite. I'll try to find the root cause.

v923z commented 2 years ago

@jimmo OK, I think I have found the commit that breaks the formatting. https://github.com/micropython/micropython/commit/6cd2e4191803e95580bdfc57c06ea818454a25d1 still produces the expected results, but https://github.com/micropython/micropython/commit/6f4d424f461bede1afb69637763f4fce5f27cd90 doesn't. However, the following snippet in python returns what the second commit does (the test script fails with the byteswap function):

from struct import pack, unpack

b = pack('d', 1.0)
a = bytes(b[::-1])
unpack('d', a)

so I must conclude that the second commit is correct. Could you, please, simply update the expected file, and also change the version number to 6.0.0 in https://github.com/v923z/micropython-ulab/blob/57de23c1fb434ba99aaafe1d00bd77d5cdf5d66b/code/ulab.c#L36? Then this could be merged immediately. Many thanks!

@jepler @tannewt @dhalbert Does circuitpython want to cherry-pick https://github.com/micropython/micropython/commit/6f4d424f461bede1afb69637763f4fce5f27cd90? Otherwise, we would have to remove byteswap from the tests, because it will fail with either circuitpython, or micropython.

@iabdalkader v1.19 is older than the commit I refer to, so one of the test scripts is going to fail, if you stay with micropython v1.19, but take ulab 5.1.1.

iabdalkader commented 2 years ago

v1.19 is older than the commit I refer to, so one of the test scripts is going to fail, if you stay with micropython v1.19, but take ulab 5.1.1.

Thanks for explaining the issue in detail, I don't think I need to run the test scripts, if I understand the issue correctly it should be fine, but I can also cherry-pick commits in v1.19 if needed.

jimmo commented 2 years ago

@v923z

so I must conclude that the second commit is correct.

Yes there has been extensive work on improving float formatting to match CPython.

Could you, please, simply update the expected file

Unfortunately not so simple...

Because build.sh tests double-precision as well as single-precision-nanbox, the results will be different, so it's not possible to write a methods.py.exp that works for both. The only way to do it is to make the results print in {:.1e}, but to be honest I'm not really sure I see the value in testing byteswap on floats.

In addition to this test there seems to be more places where the rounding matters between the regular and single-precision+nanbox runs, e.g. gc.py, and numericals.py.

jimmo commented 2 years ago

@v923z I have confirmed that the mp_obj_type_t fixes make the regular tests pass (with a small fix to gc.py that was needed anyway), so I don't think there's any regression introduced by this PR.

I'm not quite sure how the single-precision-nanbox tests passed before... it's seems amazing that the results came out exactly the same (even before the formatting fixes). I'm not really sure... with both v1.19.1 or 1.18 gc.py doesn't pass on the double precision build:

--- gc.py.exp   2022-09-22 01:50:42.228835211 +1000
+++ gc.py.out   2022-09-22 01:50:42.228835211 +1000
@@ -1,4 +1,4 @@
-988.0
+988.0000000000001
 array([1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0], dtype=float64)
-988.0
+988.0000000000001
 array([1.0, 1.0, 1.0, ..., 1.0, 1.0, 1.0], dtype=float64)

So probably the best thing instead is if you can get the tests passing against the MicroPython commit just before the mp_obj_type_t changes (i.e. https://github.com/micropython/micropython/commit/ca51d63c37e6ca67bec0a645e2aeea71aba91058) and then I can rebase this PR on top of that. Otherwise you're welcome to cherry pick the main commit from this PR.

Or I guess you could just merge this and sort out the tests afterwards?

v923z commented 2 years ago

Or I guess you could just merge this and sort out the tests afterwards?

Yes, that's what I did. Many thanks for your contribution!