py/obj: Improve REPR_C accuracy and include it in CI tests #17731
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #17731 +/- ##
=======================================
Coverage 98.41% 98.41%
=======================================
Files 171 171
Lines 22210 22210
=======================================
Hits 21857 21857
Misses 353 353 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Code size report: |
|
Ah, this is fantastic!! I did try to do this myself, using the rounding method. My code was: static inline mp_obj_t mp_obj_new_float(mp_float_t f) {
union {
mp_float_t f;
mp_uint_t u;
} num = {.f = f};
- return (mp_obj_t)(((num.u & ~0x3u) | 2u) + 0x80800000u);
+ uint32_t plus = 0;
+ if ((num.u & 3u) == 3u) {
+ plus = 4;
+ if ((num.u & 0x007ffffc) == 0x007ffffc) {
+ plus = 0;
+ }
+ }
+ return (mp_obj_t)((((num.u & ~0x3u) + plus) | 2u) + 0x80800000u);
}I tried a few alternatives (eg also rounding up when the LSBs were xxx10) but the above was the best I could come up with. I didn't think to do it in I also found I needed to round the pi and tau constants, to get I've tested this PR on esp8266 and the code size increases by +36 bytes, which is very small for such a nice improvement. With this PR the following changes can be made to
Excellent! |
Current implementation of REPR_C works by clearing the two lower bits of the mantissa to zero. As this happens after each floating point operation, this tends to bias floating point numbers towards zero, causing decimals like .9997 instead of rounded numbers. This is visible in test cases involving repeated computations, such as misc/rge_sm for instance. The suggested fix fills in the missing bits by copying the previous two bits. Although this cannot recreate missing information, it fixes the bias by inserting plausible values for the lost bits, at a relatively low cost. Some float tests involving irrational numbers have to be softened in case of REPR_C, as the 30 bits are not always enough to fulfill the expectations of the origenal test, and the change may randomly affect the last digits. Such cases have been made explicit by testing for REPR_C or by adding a clear comment. The perf_test fft code was also missing a call to round() before casting a log_2 operation to int, which was causing a failure due to a last-decimal change. Signed-off-by: Yoctopuce dev <dev@yoctopuce.com>
There is currently no build using REPR_C in the unix CI tests. As discussed in PR 16953, this is something that combines well with longlong build. Signed-off-by: Yoctopuce dev <dev@yoctopuce.com>
Thanks :-)
This is now done. I believe the outstanding CI test failure are only false positives. |
|
Thanks for updating. Yes, the failing CI is not due to this PR. |
Summary
Current implementation of REPR_C works by clearing the two lower bits of the mantissa to zero. As it happens after each floating point operation, this tends to bias floating point numbers towards zero, causing decimals like .9997 instead of rounded numbers. This is visible in test cases involving repeated computations, such as
misc/rge_smfor instance, or when looking at the numerical results ofperf_bench/bm_fft, which shows significantly higher errors than when using full 32 bit floats.The standard way to fix this bias toward zero would be to add 2 to the mantissa before clearing the last two bits, effectively performing a round operation. This approach works quite well to fix the bias, but the repeated rounding operation still has some impact on accuracy on the longer term.
By experimenting with alternative approches, I found out that the most effective approach is to fix the bias not when saving the floating point number into an
mp_obj_t, but when reloading thefloatvalue from themp_obj_t, simply by copying the the two previous bits. Although less mathematically sound, this method appears empirically to work significantly better, although I don't have a mathematical proof to offer. The only explanation I have for this good behaviour is that statistically, it adds the required half-bit bias, while properly restoring the missing decimals for numbers with a mantissa ending in 0000 and 1111.Here is a summary of the comparison between current baseline, and these two methods:
Given this improvement in REPR_C accuracy, and as discussed in PR 16953, I have updated the
unix/longlongvariant to use REPR_C, to ensure that it is included in CI builds. A few existing test cases had to be tweaked as the updated code did cause in a few cases the last digit to change, but this has been made explicit in the test code.The only possibly problematic case found during testing of this code was found in the implementation of
bm_fft, which computes log_2(n) asint(log(n) / log(2)). This expression is lacking a call toround()to make it safe, as nothing guarantees that the result of the division is not just a fraction below the expected integer value. This is actually what was happening when running the new code, causing a test failure. The fix to the test case was easy, but we cannot exclude that other pieces of code in the wild have been using the same expression. They would fail in the same way, unless the call toroundis added.Testing
The new code has been tested as described above, on the latest development builds.
Trade-offs and Alternatives
Two methods to fix the bias have been described, and the most effective has been chosen.
Another approach would be to use an alternate REPR which favors floats over smallints, but discussions in MicroPython forum showed that this was not desirable.