py/formatfloat: Format ALL whole-number floats without decimal cruft. #8905
Conversation
|
I'm happy with the code here, but it's failing the float_parse_doubleprec test for unix port / nanbox. Any advice on how I can debug this? Also, I'd be interested to see the code size comparisons. |
|
This is the size-diff I get for this PR: |
You'll need to build the unix port with The thing that fails is formatting the following numbers (represented here as bytes objects so that parsing is not part of the test): import array
print(array.array('d', b'Zb\xd7\xd7\x18\xe7ti'))
print(array.array('d', b'}\xc3\x94%\xadI\xb2T'))The result on CPython and unix But on |
|
Thanks for the help. Unfortunately, VARIANT=nanbox doesn't link on my dev system (MacOS 10.15.17, "warning: The i386 architecture is deprecated for macOS") although I can try it on my linode. Do you happen to know what is different about nanbox? It's as if it's failing some fairly specific floating-point comparisons. I tried Googling nanbox, and I understand the concept of nan-boxing, but I'm not clear how it's used in MicroPython, or why it would affect non-NaN floating point operations. |
Codecov Report
@@ Coverage Diff @@
## master #8905 +/- ##
=======================================
Coverage 98.33% 98.34%
=======================================
Files 157 157
Lines 20348 20360 +12
=======================================
+ Hits 20010 20023 +13
+ Misses 338 337 -1
Continue to review full report at Codecov.
|
|
Hmm, I was so sure I had fixed it! It turns out the "difference" may lie partially in the parsing code rather than the formatting code. On the plain build on my Mac, However, it's also true that the test case you sent (corresponding to the nanbox value, Overall, I don't think the test failure is a "bug" per se, it's just an imprecision. And the fact that it passed under the old code was .. fluke? If we changed the print in the test to use 15 sig figs, I believe it would work. But asking you to change the test to pass my code feels .. problematic. |
Yes I'm also seeing this behaviour, that two values which have the same binary representation are not comparing equal. I think the issue is that on x86 the internal precision of double operations is 80 bits. And so even though they are equivalent 64-bit doubles, their 80-bit representations differ. I could fix it by forcing a truncation of the 80-bit number: @@ -265,9 +270,10 @@ int mp_format_float(FPTYPE f, char *buf, size_t buf_size, char fmt, int prec, ch
// mantissa.
FPTYPE u_base = FPCONST(1.0);
for (e = 0, e1 = FPDECEXP; e1; e1 >>= 1, pos_pow++) {
- if (f >= (u_base * *pos_pow)) {
+ volatile double xx = u_base * *pos_pow;
+ if (f >= xx) {
e += e1;
- u_base *= *pos_pow;
+ u_base = xx;
}
}That's not very satisfactory. Can you change the Alternatively, go back to the integer version #8901. |
|
I tried testing for the equality case separately, after the initial loop, by subtracting the new base 10 times and seeing if the result was >= 0, but it failed the same way - again, presumably, because the compiler is optimizing by keeping the intermediate result in an 80 bit register. The equality case is crucial here, because it's the difference between trying to format 1e100 as 1e100 vs. 9.99999..e99. It turns out that the cumulative errors when printing 9.9999..e99 to 16 significant figures are such that it doesn't round up to 1e100 (because the value "looks like" 9.9999999999999987334e+199, so rounds up at 16 sf to 9.9999...). Given that we're now coding around deviations from the ideal model, I think "volatile" is as good a solution as any - at least the reader knows that it's not about details of the math, but something in the compilation. It's so interesting that this only came up for nanbox. I assume it's non-IEEE-compliant to do comparisons without truncating down to 64 bits; maybe the deal with nanbox is that it's already noncompliant, so they pull in other optimizations too? |
dea3d5f to
7e5dc50
Compare
|
I got a little deeper into the code and simplified it, looking for code size reductions. |
Nan-boxing mode is defined at our (MicroPython) end, it's a 32-bit x86 build with 64-bit objects. The same bug appears in normal 32-bit builds of unix MicroPython. The reason the CI didn't see it (which does build and test 32-bit unix MicroPython) is because the parsing of numbers is slightly different in nanbox vs non-nanbox (as noted up above). You can see the error on a standard 32-bit build by using the test that gives the explicit representation of the double: import array
print(array.array('d', b'Zb\xd7\xd7\x18\xe7ti'))
print(array.array('d', b'}\xc3\x94%\xadI\xb2T'))We should definitely add this exact test to the test suite. If we go the route using |
|
Current code size diff is now: That's surprising that unix is exactly 0... I double checked it and it seems to be true! |
py/formatfloat.c
Outdated
| // Use "volatile" to force the result to be stored in a float64. | ||
| // Without this, nanbox build would sometimes fail to report | ||
| // "equality" leading to weird results. | ||
| volatile FPTYPE next_u = u_base * *pos_pow; |
There was a problem hiding this comment.
Please guard the volatile with #if defined(__i386__)
| # represented by a float32 and that will also fit within a (signed) int32. | ||
| # The upper bound of our integer-handling code is actually double this, | ||
| # but that constant might cause trouble on systems using 32 bit ints. | ||
| print("{:f}".format(2147483520)) |
There was a problem hiding this comment.
Please add the 2 test cases using the explicit representation and array.array
There was a problem hiding this comment.
Won't that fail on builds with
#define MICROPY_FLOAT_IMPL (MICROPY_FLOAT_IMPL_FLOAT)
I guess there's some way to gate or condition tests based on build conditions. I'll look into it.
There was a problem hiding this comment.
I'm going with a separate tests/float/float_format_int_doubleprec.py, which I'll add to the if upy_float_precision < 64 skips in run-tests.py.
3defa4d to
9a929b2
Compare
|
This looks good, right? I regret to say there are still some integer powers of ten that don't work out perfectly, e.g. (33 is the smallest, and there are only 10 below 1e100). I'm not sure how this is coming about, I guess because there are differences in how the parsing constructs the float, and how we build it in the formatting code (by multiplying-up factors of 10**(2**i)). The unmodified code does this too, with the lowest being at 1e23. However, there are only a total of 7 problem cases below 1e100. |
0a2a26e to
d4499d9
Compare
|
I was able to fix printing of all values of 1eX by implementing The consequence is that some values that are legitimately just smaller than 10^X are now rounded up too aggressively. However, the closest this comes to affecting integers is around 10^16: Even at this point, we can't exactly represent every integer since eps=2. However, we can exactly represent 10^16 - 2, but instead it gets rounded up to 10^16. It's not perfect, but I think it's OK as the price we pay for clean printing of 10^X values. I added a test of printing all values of 10^X up to 10^32 (largest that remains finite in float32). This worked at 12 significant figures on all architectures except qemu-arm, which for some reason had serious problems with 10^11. I had to reduce the resolution to 7 s.f. to make it pass. |
|
(oh and the |
|
To illustrate the 'aggressive rounding up' problem more completely, consider the following code fragment: Notice the jump from However, compare the output from the current It manages to nail the 10^16 value (as it happens, although it doesn't for 10^23, 10^25, or 10^29), but all those other non-integer values - ugh. |
|
Great. I rebased to master, changed to using |
|
Running the new tests on bare-metal targets shows a few issues:
Otherwise, the rest of the tests pass on these two boards (and they all pass on PYBD_SF6, which is stm32 with hardware double precision floats). We need to get these tests passing on these bare-metal targets. For the first case could it use |
|
New size diff is slightly better than before: |
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. This change avoids scaling until the value is smaller than 1, so all whole numbers print with zero fractional part. This is to fix issue micropython#4212 Signed-off-by: Dan Ellis dan.ellis@gmail.com
|
|
I'm not sure how passionate we feel about code size vs. execution speed. I've continued playing with this (after looking at |
|
I have a |
|
If you want to make a PR for that change, then we can evaluate it. |
Formerly, ```formatfloat``` struggled with formatting exact powers of 10 as created by ```parsenum``` because of small differences in how those powers of 10 were calculated: ```formatfloat``` multiplied together a few values of 10^(2^Y) from a lookup table, but ```parsenum``` used ```pow(10, X)```. This was mitigated in micropython#8905 by adding 2eps of extra margin when comparing values ("aggressive rounding up"). This patch instead eliminates the discrepency by using ```pow()``` in ```formatfloat``` also. This eliminates the need for the 2eps margin, as well as the lookup tables. It is likely slower to run, however. In addition, this patch directly estimates the power-of-10 exponent from the power-of-2 exponent in the floating-point representation. This change surfaced a previously-noted problem with parsing numbers including trailing zeros. The fix to that problem, micropython#8980, is included in this PR to ensure the tests pass. Signed-off-by: Dan Ellis <dan.ellis@gmail.com>
Formerly, ```formatfloat``` struggled with formatting exact powers of 10 as created by ```parsenum``` because of small differences in how those powers of 10 were calculated: ```formatfloat``` multiplied together a few values of 10^(2^Y) from a lookup table, but ```parsenum``` used ```pow(10, X)```. This was mitigated in micropython#8905 by adding 2eps of extra margin when comparing values ("aggressive rounding up"). This patch instead eliminates the discrepency by using ```pow()``` in ```formatfloat``` also. This eliminates the need for the 2eps margin, as well as the lookup tables. It is likely slower to run, however. In addition, this patch directly estimates the power-of-10 exponent from the power-of-2 exponent in the floating-point representation. This change surfaced a previously-noted problem with parsing numbers including trailing zeros. The fix to that problem, micropython#8980, is included in this PR to ensure the tests pass. Signed-off-by: Dan Ellis <dan.ellis@gmail.com>
Formerly, ```formatfloat``` struggled with formatting exact powers of 10 as created by ```parsenum``` because of small differences in how those powers of 10 were calculated: ```formatfloat``` multiplied together a few values of 10^(2^Y) from a lookup table, but ```parsenum``` used ```pow(10, X)```. This was mitigated in micropython#8905 by adding 2eps of extra margin when comparing values ("aggressive rounding up"). This patch instead eliminates the discrepency by using ```pow()``` in ```formatfloat``` also. This eliminates the need for the 2eps margin, as well as the lookup tables. It is likely slower to run, however. In addition, this patch directly estimates the power-of-10 exponent from the power-of-2 exponent in the floating-point representation. This change surfaced a previously-noted problem with parsing numbers including trailing zeros. The fix to that problem, micropython#8980, is included in this PR to ensure the tests pass. Signed-off-by: Dan Ellis <dan.ellis@gmail.com>
Formerly, ```formatfloat``` struggled with formatting exact powers of 10 as created by ```parsenum``` because of small differences in how those powers of 10 were calculated: ```formatfloat``` multiplied together a few values of 10^(2^Y) from a lookup table, but ```parsenum``` used ```pow(10, X)```. This was mitigated in micropython#8905 by adding 2eps of extra margin when comparing values ("aggressive rounding up"). This patch instead removes the discrepency by using ```pow()``` in ```formatfloat``` also. This eliminates the need for the 2eps margin, as well as the lookup tables. It is likely slower to run, however. In addition, this patch directly estimates the power-of-10 exponent from the power-of-2 exponent in the floating-point representation. This change surfaced a previously-noted problem with parsing numbers including trailing zeros. The fix to that problem, micropython#8980, is included in this PR to ensure the tests pass. Signed-off-by: Dan Ellis <dan.ellis@gmail.com>
Formerly, ```formatfloat``` struggled with formatting exact powers of 10 as created by ```parsenum``` because of small differences in how those powers of 10 were calculated: ```formatfloat``` multiplied together a few values of 10^(2^Y) from a lookup table, but ```parsenum``` used ```pow(10, X)```. This was mitigated in micropython#8905 by adding 2eps of extra margin when comparing values ("aggressive rounding up"). This patch instead removes the discrepency by using ```pow()``` in ```formatfloat``` also. This eliminates the need for the 2eps margin, as well as the lookup tables. It is likely slower to run, however. In addition, this patch directly estimates the power-of-10 exponent from the power-of-2 exponent in the floating-point representation. This change surfaced a previously-noted problem with parsing numbers including trailing zeros. The fix to that problem, micropython#8980, is included in this PR to make the tests pass. Signed-off-by: Dan Ellis <dan.ellis@gmail.com>
Formerly, ```formatfloat``` struggled with formatting exact powers of 10 as created by ```parsenum``` because of small differences in how those powers of 10 were calculated: ```formatfloat``` multiplied together a few values of 10^(2^Y) from a lookup table, but ```parsenum``` used ```pow(10, X)```. This was mitigated in micropython#8905 by adding 2eps of extra margin when comparing values ("aggressive rounding up"). This patch instead removes the discrepency by using ```pow()``` in ```formatfloat``` also. This eliminates the need for the 2eps margin, as well as the lookup tables. It is likely slower to run, however. In addition, this patch directly estimates the power-of-10 exponent from the power-of-2 exponent in the floating-point representation. This change surfaced a previously-noted problem with parsing numbers including trailing zeros. The fix to that problem, micropython#8980, is included in this PR to make the tests pass. Signed-off-by: Dan Ellis <dan.ellis@gmail.com>
Formerly, ```formatfloat``` struggled with formatting exact powers of 10 as created by ```parsenum``` because of small differences in how those powers of 10 were calculated: ```formatfloat``` multiplied together a few values of 10^(2^Y) from a lookup table, but ```parsenum``` used ```pow(10, X)```. This was mitigated in micropython#8905 by adding 2eps of extra margin when comparing values ("aggressive rounding up"). This patch instead removes the discrepency by using ```pow()``` in ```formatfloat``` also. This eliminates the need for the 2eps margin, as well as the lookup tables. It is likely slower to run, however. In addition, this patch directly estimates the power-of-10 exponent from the power-of-2 exponent in the floating-point representation. This change surfaced a previously-noted problem with parsing numbers including trailing zeros. The fix to that problem, micropython#8980, is included in this PR to make the tests pass. Signed-off-by: Dan Ellis <dan.ellis@gmail.com>
Formerly, ```formatfloat``` struggled with formatting exact powers of 10 as created by ```parsenum``` because of small differences in how those powers of 10 were calculated: ```formatfloat``` multiplied together a few values of 10^(2^Y) from a lookup table, but ```parsenum``` used ```pow(10, X)```. This was mitigated in micropython#8905 by adding 2eps of extra margin when comparing values ("aggressive rounding up"). This patch instead removes the discrepency by using ```pow()``` in ```formatfloat``` also. This eliminates the need for the 2eps margin, as well as the lookup tables. It is likely slower to run, however. In addition, this patch directly estimates the power-of-10 exponent from the power-of-2 exponent in the floating-point representation. This change surfaced a previously-noted problem with parsing numbers including trailing zeros. The fix to that problem, micropython#8980, is included in this PR to make the tests pass. Signed-off-by: Dan Ellis <dan.ellis@gmail.com>
Formerly, ```formatfloat``` struggled with formatting exact powers of 10 as created by ```parsenum``` because of small differences in how those powers of 10 were calculated: ```formatfloat``` multiplied together a few values of 10^(2^Y) from a lookup table, but ```parsenum``` used ```pow(10, X)```. This was mitigated in micropython#8905 by adding 2eps of extra margin when comparing values ("aggressive rounding up"). This patch instead removes the discrepency by using ```pow()``` in ```formatfloat``` also. This eliminates the need for the 2eps margin, as well as the lookup tables. It is likely slower to run, however. In addition, this patch directly estimates the power-of-10 exponent from the power-of-2 exponent in the floating-point representation. This change surfaced a previously-noted problem with parsing numbers including trailing zeros. The fix to that problem, micropython#8980, is included in this PR to make the tests pass. Signed-off-by: Dan Ellis <dan.ellis@gmail.com>
Formerly, ```formatfloat``` struggled with formatting exact powers of 10 as created by ```parsenum``` because of small differences in how those powers of 10 were calculated: ```formatfloat``` multiplied together a few values of 10^(2^Y) from a lookup table, but ```parsenum``` used ```pow(10, X)```. This was mitigated in micropython#8905 by adding 2eps of extra margin when comparing values ("aggressive rounding up"). This patch instead removes the discrepency by using ```pow()``` in ```formatfloat``` also. This eliminates the need for the 2eps margin, as well as the lookup tables. It is likely slower to run, however. In addition, this patch directly estimates the power-of-10 exponent from the power-of-2 exponent in the floating-point representation. This change surfaced a previously-noted problem with parsing numbers including trailing zeros. The fix to that problem, micropython#8980, is included in this PR to make the tests pass. Signed-off-by: Dan Ellis <dan.ellis@gmail.com>
Formerly, ```formatfloat``` struggled with formatting exact powers of 10 as created by ```parsenum``` because of small differences in how those powers of 10 were calculated: ```formatfloat``` multiplied together a few values of 10^(2^Y) from a lookup table, but ```parsenum``` used ```pow(10, X)```. This was mitigated in micropython#8905 by adding 2eps of extra margin when comparing values ("aggressive rounding up"). This patch instead removes the discrepency by using ```pow()``` in ```formatfloat``` also. This eliminates the need for the 2eps margin, as well as the lookup tables. It is likely slower to run, however. In addition, this patch directly estimates the power-of-10 exponent from the power-of-2 exponent in the floating-point representation. This change surfaced a previously-noted problem with parsing numbers including trailing zeros. The fix to that problem, micropython#8980, is included in this PR to make the tests pass. Signed-off-by: Dan Ellis <dan.ellis@gmail.com>
Formerly, ```formatfloat``` struggled with formatting exact powers of 10 as created by ```parsenum``` because of small differences in how those powers of 10 were calculated: ```formatfloat``` multiplied together a few values of 10^(2^Y) from a lookup table, but ```parsenum``` used ```pow(10, X)```. This was mitigated in micropython#8905 by adding 2eps of extra margin when comparing values ("aggressive rounding up"). This patch instead removes the discrepency by using ```pow()``` in ```formatfloat``` also. This eliminates the need for the 2eps margin, as well as the lookup tables. It is likely slower to run, however. In addition, this patch directly estimates the power-of-10 exponent from the power-of-2 exponent in the floating-point representation. This change surfaced a previously-noted problem with parsing numbers including trailing zeros. The fix to that problem, micropython#8980, is included in this PR to make the tests pass. Signed-off-by: Dan Ellis <dan.ellis@gmail.com>
Formerly, ```formatfloat``` struggled with formatting exact powers of 10 as created by ```parsenum``` because of small differences in how those powers of 10 were calculated: ```formatfloat``` multiplied together a few values of 10^(2^Y) from a lookup table, but ```parsenum``` used ```pow(10, X)```. This was mitigated in micropython#8905 by adding 2eps of extra margin when comparing values ("aggressive rounding up"). This patch instead removes the discrepency by using ```pow()``` in ```formatfloat``` also. This eliminates the need for the 2eps margin, as well as the lookup tables. It is likely slower to run, however. In addition, this patch directly estimates the power-of-10 exponent from the power-of-2 exponent in the floating-point representation. This change surfaced a previously-noted problem with parsing numbers including trailing zeros. The fix to that problem, micropython#8980, is included in this PR to make the tests pass. Signed-off-by: Dan Ellis <dan.ellis@gmail.com>
Formerly, ```formatfloat``` struggled with formatting exact powers of 10 as created by ```parsenum``` because of small differences in how those powers of 10 were calculated: ```formatfloat``` multiplied together a few values of 10^(2^Y) from a lookup table, but ```parsenum``` used ```pow(10, X)```. This was mitigated in micropython#8905 by adding 2eps of extra margin when comparing values ("aggressive rounding up"). This patch instead removes the discrepency by using ```pow()``` in ```formatfloat``` also. This eliminates the need for the 2eps margin, as well as the lookup tables. It is likely slower to run, however. In addition, this patch directly estimates the power-of-10 exponent from the power-of-2 exponent in the floating-point representation. This change surfaced a previously-noted problem with parsing numbers including trailing zeros. The fix to that problem, micropython#8980, is included in this PR to make the tests pass. Signed-off-by: Dan Ellis <dan.ellis@gmail.com>
Jpegio fixes: swap order of size and example code Argument
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. This
change avoids scaling until the value is smaller than 1, so
all whole numbers print with zero fractional part.
This is to fix issue #4212.
This PR is an alternative to #8901. It uses a different algorithm, using
floating-point arithmetic instead of uint32_t. This makes it able to
work for all whole values (not only those smaller than 2^32). It may
also reduce code size (to be verified).
Signed-off-by: Dan Ellis dan.ellis@gmail.com