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

Floats print with incorrect rounding in compact context #36234

Closed
rdeits opened this issue Jun 11, 2020 · 12 comments
Closed

Floats print with incorrect rounding in compact context #36234

rdeits opened this issue Jun 11, 2020 · 12 comments
Assignees
Labels
bug Indicates an unexpected problem or unintended behavior display and printing Aesthetics and correctness of printed representations of objects. regression Regression in behavior compared to a previous version
Milestone

Comments

@rdeits
Copy link
Contributor

rdeits commented Jun 11, 2020

Similar to #33185 but present in 1.4.2 and 1.5-beta1.

Originally from: https://discourse.julialang.org/t/incorrect-display-of-array-float64-2-numbers/41200/3

MWE:

julia> sprint(show, -498796.2749933266, context=:compact=>true)
"-498796.0"

The .0 is wrong--it ought to be .3.

@rdeits
Copy link
Contributor Author

rdeits commented Jun 11, 2020

This appears to be an edge case for floats with exactly 6 digits before the decimal place:

julia> sprint(show, 12.345678, context=:compact=>true)
"12.3457"

julia> sprint(show, 123.45678, context=:compact=>true)
"123.457"

julia> sprint(show, 1234.5678, context=:compact=>true)
"1234.57"

julia> sprint(show, 12345.678, context=:compact=>true)
"12345.7"

julia> sprint(show, 123456.78, context=:compact=>true)  # Incorrect
"123457.0"

julia> sprint(show, 1234567.8, context=:compact=>true)
"1.23457e6"

@rdeits
Copy link
Contributor Author

rdeits commented Jun 11, 2020

I guess I see what's happening here: the value is correctly rounded to 123457, but because it's still a Float64, it gets printed as 123457.0. That gives the impression that the .0 is a valid significant digit, but it's actually just signifying the type of the result.

@JeffBezanson
Copy link
Member

Ah, I guess it should be printed as -498796.?

@JeffBezanson JeffBezanson added bug Indicates an unexpected problem or unintended behavior display and printing Aesthetics and correctness of printed representations of objects. labels Jun 11, 2020
@rdeits
Copy link
Contributor Author

rdeits commented Jun 11, 2020

Or perhaps the compact display logic should avoid rounding to 0 digits? It doesn't save any space to do so, since the .0 will be appended anyway.

@vtjnash
Copy link
Member

vtjnash commented Jun 11, 2020

I seem to recall fixing this in the old Grisu code (#6608 and #9072), so that in v0.6 we had this output:

julia> [-498796.2749933266 1]
1×2 Array{Float64,2}:
 -4.98796e5  1.0

(commit 1890e57)

@vtjnash
Copy link
Member

vtjnash commented Jun 11, 2020

Ah, we even had a test for this (5d2a0ec), but it was deleted when switching from Grisu->Ryu (869090b)

@vtjnash vtjnash added the regression Regression in behavior compared to a previous version label Jul 13, 2020
@vtjnash vtjnash added this to the 1.6 milestone Jul 13, 2020
@JeffBezanson
Copy link
Member

Bump @quinnj --- I doubt there is time for this in 1.5 but this seems pretty bad.

@JeffreySarnoff
Copy link
Contributor

rather than the current incorrect

julia> sprint(show, 12345.678, context=:compact=>true)
"12345.7"

julia> sprint(show, 123456.78, context=:compact=>true)  # Incorrect
"123457.0"

julia> sprint(show, 1234567.8, context=:compact=>true)
"1.23457e6"

why not re-adopt what @vtjnash had done

julia> sprint(show, 12345.678, context=:compact=>true)
"12345.7"

julia> sprint(show, 123456.78, context=:compact=>true)  # Correct and nicer than "123457."
"1.23457e5"

julia> sprint(show, 1234567.8, context=:compact=>true)
"1.23457e6"

@JeffreySarnoff
Copy link
Contributor

JeffreySarnoff commented Jul 27, 2020

to obtain

julia> sprint(show, 12345.678, context=:compact=>true)
"12345.7"

julia> sprint(show, 123456.78, context=:compact=>true)
"1.23457e5"

julia> sprint(show, 1234567.8, context=:compact=>true)
"1.23457e6"

fix Line 333 of Ryu/shortest.jl

    if -4 < pt <= (precision == -1 ? (T == Float16 ? 3 : 6) : precision)

by replacing <= with <

    if -4 < pt < (precision == -1 ? (T == Float16 ? 3 : 6) : precision)

@JeffreySarnoff
Copy link
Contributor

Well, it appears there are other impinging considerations. So I closed the PR. Someone more familiar with the other uses of that line ...

@quinnj
Copy link
Member

quinnj commented Jul 27, 2020

Fix is up: #36819

@quinnj
Copy link
Member

quinnj commented Jul 27, 2020

Fixed via #36819

@quinnj quinnj closed this as completed Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior display and printing Aesthetics and correctness of printed representations of objects. regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants