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

circuitpython and micropython tests deliver differing results #541

Closed v923z closed 2 years ago

v923z commented 2 years ago

https://github.com/micropython/micropython/commit/f9cbe6bc47dd4f5b8e85178caecd6f0de22b4c34 modifies the formatting of certain floats, while circuitpython keeps the old implementation. This difference leads to differing outputs on the test scripts.

@jepler @tannewt @dhalbert What would be the preferable fix for this till circuitpython pulls in the new implementation? I would not like to have two tiers of tests, but I also undrestand that ulab should be tested against circuitpython, too.

@jepler By assigning the issue to you I don't want to imply that you have to fix it, but I wanted to have someone from that side.

dhalbert commented 2 years ago

I think we could cherry-pick the formatting fix for 8.0.0, and ignore it for 7.3.x. It will not be an issue until we try to update ulab.

v923z commented 2 years ago

@dhalbert Thanks for the feedback! I could take it out of the CI for the time being. We just have to make sure that we put it back, once you merged the float formatting. Would it be possible to add this to the 8.0.0 milestone, so that we don't forget about it?

dhalbert commented 2 years ago

Oh, I didn't realize it was being tested in your CI. I wonder if you can make the tests be rather more forgiving (limiting significant digits), or are there formatting changes all over the place?

v923z commented 2 years ago

This issue pops up, whenever the result of whatever operation is an integer float. There are several test scripts that produce that. Here is an example: https://github.com/v923z/micropython-ulab/runs/7528072036?check_suite_focus=true

dhalbert commented 2 years ago

In that example, is 988.0000000000001 the value that CircuitPython currently produces or that the newly updated MicroPython produces?

v923z commented 2 years ago

This is the value produced by circuitpython. micropython now prints just 998.0.

v923z commented 2 years ago

I haven't looked at the implementation in detail, but the commit message is 

Formerly, py/formatfloat would print whole numbers inaccurately with
nonzero digits beyond the decimal place.  This resulted from its strategy
of successive scaling of the argument by 0.1 which cannot be exactly
represented in floating point.
dhalbert commented 2 years ago

The new fix is a good one, I just wanted to double-check.

dpgeorge commented 2 years ago

In evaluating that improvement to formatfloat, I did run it on the ulab test suite. IMO the output of ulab tests is much better with this formatfloat fix.

v923z commented 2 years ago

I definitely agree, this is a significant improvement. Thanks for merging it!

v923z commented 2 years ago

The issue is chalked up in https://github.com/adafruit/circuitpython/issues/6649, so this can be closed.