Skip to content

py/parsenum: fix rounding error when float ends in .0 #5832

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

Closed
wants to merge 1 commit into from

Conversation

dlech
Copy link
Contributor

@dlech dlech commented Mar 28, 2020

It is common code style to write floating point numbers with trailing .0 instead of just trailing .. When there is a trailing .0, the current parser will multiply the number by 10 and then divide it by 10 again later. For certain numbers, this can cause rounding.

This adds a check for the special case of .0 to avoid the extra work. Even in cases where there is not a rounding problem, this should be more efficient, especially for soft float implementations.

Fixes #5831

@dlech dlech mentioned this pull request Mar 28, 2020
@dlech dlech force-pushed the float-bug branch 2 times, most recently from c8e8da6 to 62a9539 Compare March 28, 2020 22:22
It is common code style to write floating point numbers with trailing
`.0` instead of just trailing `.`. When there is a trailing `.0`, the
current parser will multiply the number by 10 and then divide it by
10 again later. For certain numbers, this can cause rounding.

This adds a check for the special case of `.0` to avoid the extra work.
Also it avoids a call to pow() and extra multiplication when the exp_val
is 0. Even in cases where there is not a rounding problem, this should
be more efficient, especially for soft float implementations.
@stinos
Copy link
Contributor

stinos commented Mar 29, 2020

How about checking all digits for 0 as you suggest in the issue? Then the behavior is correct without edge cases.

@dlech
Copy link
Contributor Author

dlech commented Mar 29, 2020

I thought about it, but performance and code size also have to be taken into consideration. Since there are no instances in micropython and micropython-lib where x.00 is used, but if black is used to format code there will be many instances of x.0, this seems like a good compromise. Of course, I'm glad to change it if the consensus is different from my opinion.

@dhalbert
Copy link
Contributor

dhalbert commented Mar 29, 2020

I would request that you check for any number of 0's. x.00 being different than x.0 is not something I would want to explain to the average user, and they would certainly think it very odd. I'm speaking from a CircuitPython perspective: we try to avoid decisions that we know are going to be a support issue.

(Sometimes I use this criterion: am I going to spend a lot more time explaining this and are people going to spend a lot more time debugging something than the CPU time that would be saved?)

In terms of performance, I think the check will be swamped by the rest of the conversion, and I don't think the code size increase would be significant.

I think the whole conversion algorithm could use revisiting, but there are several other issues on that topic.

@dlech
Copy link
Contributor Author

dlech commented Jun 4, 2021

superseded by #6024

@dlech dlech closed this Jun 4, 2021
tannewt added a commit to tannewt/circuitpython that referenced this pull request Jan 20, 2022
@dpwe
Copy link
Contributor

dpwe commented Jul 28, 2022

n.b. this remains an issue:

>>> print(1e-37 - 1.000000000000e-37)
2.088097429759528e-53

.. because 1e-37 is calculated as 1 * pow(10, -37), whereas 1.000000000000e-37 is calculated as 1000000000000 * pow(10, -49), which comes out as 1eps different (on x64 FPU).

@dpgeorge
Copy link
Member

This was never merged. It was closed in favour of #6024, which is also not merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trailing .0 in float literal can cause rounding error
5 participants