-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
py/formatfloat: Calculate reference values with pow(). #8985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
20938e0
to
2e8e515
Compare
@dpgeorge Any hints as to why this is failing on appveyor? All I can see is the build error outputs, which seem catastrophic - my code prints nothing for numbers smaller than 1. But I can't see how this is happening (and can't reproduce, obviously). The Appveyor build appears to be under Windows using an MS compiler - is there some reason why mp_float_union_t wouldn't work there? I got rid of the special case float_union_t (for accessing the bits in a 32-bit float) from formatfloat since it appeared redundant with mp_float_union_t, but now I'm doubting my actions. In particular, the new code computes directly from the exp bits extracted from mp_float_union_t, maybe these read as signed under the MS compiler or something? Edit: This was fixed by directly accessing the bits in the |
Although the tests pass, there's a big problem with this. Using a float32 build,
That's a malformed "e"-format number, the leading digit cannot be 0 in "e"-format. The problem occurs with a mismatch between how the leading digit place is calculated, and how the leading digit itself is calculated. For numbers smaller than 1.0, we decide the leading digit by comparing the input number with We then scale up the input by
Thus, when we come to see how many units we can fit in the scaled-up input, it's zero, and we print the incorrectly-normalized "e"-format number above. I think this was being hidden by the hard Edit: This was fixed by dividing by |
963fe59
to
caaf141
Compare
This code is more understandable, I believe more accurate (no aggressive rounding), and should even be smaller (although the changes to |
Code size diff for this PR (which includes #8980):
That's great! |
This looks like a good improvement, because it reduces code size and fixes the trailing 0 problem. And as you mention it no longer does the aggressive rounding up. Can you please rebase this on master and clean up the commits, with at least a separate commit to fix the trailing zero problem, and then 1 or more extra commits for the other changes. (At the moment the commits conflict, it seems it was merged to itself.) |
64d3e17
to
912334c
Compare
Rework the conversion of floats to decimal strings so it aligns precisely with the conversion of strings to floats in parsenum.c. This is to avoid rendering 1eX as 9.99999eX-1 etc. Signed-off-by: Dan Ellis <dan.ellis@gmail.com>
912334c
to
0419279
Compare
Sounds good. I have rebased and cleaned up the commit history as you requested. I have rebased to HEAD, then there are now two commits, one adds the changes to parsenum.c, and the second adds the changes to formatfloat.c and the tests. I'm getting more successful with |
Thanks for updating the commits. Rebased and merged in 6cd2e41 and 6f4d424 Final code size diff for this PR is:
|
Formerly,
formatfloat
struggled with formatting exact powers of 10as created by
parsenum
because of small differences in how thosepowers of 10 were calculated:
formatfloat
multiplied together a fewvalues of 10^(2^Y) from a lookup table, but
parsenum
usedpow(10, X)
. This was mitigated in #8905 by adding 2eps of extramargin when comparing values ("aggressive rounding up").
This patch instead eliminates the discrepency by using
pow()
informatfloat
also. This eliminates the need for the 2eps margin, aswell 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, #8980, is included
in this PR to ensure the tests pass.
Signed-off-by: Dan Ellis dan.ellis@gmail.com