Skip to content
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

Float printing with incorrect rounding. #33185

Closed
BioTurboNick opened this issue Sep 6, 2019 · 12 comments
Closed

Float printing with incorrect rounding. #33185

BioTurboNick opened this issue Sep 6, 2019 · 12 comments
Assignees
Labels
regression Regression in behavior compared to a previous version

Comments

@BioTurboNick
Copy link
Contributor

I can't say the cause, but one of my tests with respect to show() for a custom type has started breaking in the nightly Julia branch in Travis CI tests. 1.2 is fine.

The difference is in the rounding behavior for display of a floating point value using

sprint(show, 22.89825, context=:compact=>true)

This used to produce 22.8983. Now it produces 22.8982.

May be linked to switch from "Grisu" to "Ryu"? Don't know why that would be though. From commit 869090b

@quinnj
Copy link
Member

quinnj commented Sep 6, 2019

This seems to be related to how we now use round in compact mode; see:

julia> round(22.89825, sigdigits=6)
22.8982

I'm not an expert on rounding or understand why this is rounding like it is; maybe @simonbyrne or @JeffreySarnoff know what's going on here?

@simonbyrne
Copy link
Contributor

julia> @printf "%.60f" 22.89825
22.898250000000000881072992342524230480194091796875000000000000

So "22.8983" is the right answer.

We should try not to use round in printing, see the Note in the help text:
https://docs.julialang.org/en/v1/base/math/#Base.round-Tuple{Type,Any}

@quinnj
Copy link
Member

quinnj commented Sep 6, 2019

..........so round can get things wrong? I'm not sure I understand the note; it says that even though 1.15 is actually less than 1.15, it still rounds to 1.2, which seems great, it still does what you want. So why exactly does round(22.89825, sigdigits=6) round incorrectly?

@jmkuhn
Copy link
Contributor

jmkuhn commented Sep 7, 2019

If 22.89825 were exactly representable as a binary float, the new behavior would be correct. round(22.89825, sigdigits = 6) on both 1.2 and 1.4-DEV also give the desired result of 22.8982 using the default rounding mode of round to nearest ties to even. @sprintf("%.4f", 22.89825) on both return "22.8983". The difference is that 1.4-DEV now uses round in compact mode. So is this a bug fix or should the correct answer be "22.8983" since the binary representation is > 22.89825?

Julia 1.2:

julia> round(22.89825, sigdigits=6)
22.8982

julia> @sprintf("%.4f", 22.89825)
"22.8983"

julia> sprint(show, 22.89825, context=:compact=>true)
"22.8983"

Julia 1.4.0-DEV.97

julia> round(22.89825, sigdigits=6)
22.8982

julia> @sprintf("%.4f", 22.89825)
"22.8983"

julia> sprint(show, 22.89825, context=:compact=>true)
"22.8982"

@BioTurboNick
Copy link
Contributor Author

If I'm reading details of IEEE-754 right, the correct way is to treat the representable value as if it were the infinitely precise intended value. And with rounding nearest tie to even, the result in the 1.4-DEV would seem to be correct?

But for comparison, in C#:

double value = 22.89825;
Console.WriteLine(value.ToString("0.0000"));

Produces 22.8983

@simonbyrne
Copy link
Contributor

it says that even though 1.15 is actually less than 1.15, it still rounds to 1.2, which seems great, it still does what you want.

Leaving aside that "what you want" will differ between users, this isn't guaranteed, as there are cases which also go the other way, e.g.

julia> round(0.545,digits=2)
0.55

We probably should be more explicit in the docs that it may not handle near-tied values correctly (no matter how you define the "correct" behaviour). But my original point is that any printing code shouldn't be calling round at all: if we're trying to output decimal values, trying to do operations in a binary format is asking for trouble.

@simonbyrne
Copy link
Contributor

Looking at the Ryu code, it looks like the correct solution is to use writeexp instead of writeshortest?

@StefanKarpinski
Copy link
Sponsor Member

so round can get things wrong?

Yes, round can get things wrong, which needs to be fixed, but also this should not use round because that's less efficient: it means you are trying to round a binary number to another binary number which, when printed will only require a certain number of decimal digits to reproduce the binary value. It would be much more efficient to print the desired number of decimal digits.

@JeffBezanson JeffBezanson added the regression Regression in behavior compared to a previous version label Sep 10, 2019
@quinnj
Copy link
Member

quinnj commented Sep 18, 2019

Alright, diving in on this now.

quinnj added a commit that referenced this issue Sep 18, 2019
…igits instead of using round function. Note the significant digits do not include leading/trailing zeros
@quinnj quinnj closed this as completed in 0bd60e6 Sep 20, 2019
quinnj added a commit that referenced this issue Sep 20, 2019
Fix #33185 by rounding to 6 significant digits while printing float d…
@simonbyrne
Copy link
Contributor

simonbyrne commented Oct 3, 2019

It looks like there are still some cases where we aren't breaking ties correctly (these are actually broken in versions as well):

julia> f = 1+0.5^6
1.015625

julia> using Printf

julia> @printf "%.5e" big(f)
1.01562e+00

julia> @printf "%.5e" f
1.01563e+00

julia> fill(f,2,2)
2×2 Array{Float64,2}:
 1.01563  1.01563
 1.01563  1.01563

Note that Ryu.writefixed gets this correct:

julia> Base.Ryu.writefixed(f,5)
"1.01562"

@BioTurboNick
Copy link
Contributor Author

I was about to post something similar.

That bug was fixed but a new one was introduced. The value "0.025621074" is being printed as "0.025621" and not "0.0256211" in the nightly.

@BioTurboNick
Copy link
Contributor Author

Just confirming that my tests pass on the nightly now. Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Regression in behavior compared to a previous version
Projects
None yet
Development

No branches or pull requests

6 participants