Ryu engineering2

[isentropic]

(@v1.5) pkg> test Showoff
    Testing Showoff
Status `/tmp/jl_ffG9Ue/Project.toml`
  [42e2da0e] Grisu v1.0.0
  [992d4aef] Showoff v0.3.2 `~/Software/Showoff`
  [ade2ca70] Dates
  [de0858da] Printf
  [8dfed614] Test
Status `/tmp/jl_ffG9Ue/Manifest.toml`
  [42e2da0e] Grisu v1.0.0
  [992d4aef] Showoff v0.3.2 `~/Software/Showoff`
  [2a0f44e3] Base64
  [ade2ca70] Dates
  [8ba89e20] Distributed
  [b77e0a4c] InteractiveUtils
  [56ddb016] Logging
  [d6f4376e] Markdown
  [de0858da] Printf
  [9a3f8284] Random
  [9e88b42a] Serialization
  [6462fe0b] Sockets
  [8dfed614] Test
  [4ec0a83e] Unicode
Test Summary: | Pass  Total
Internals     |    7      7
Test Summary: | Pass  Total
Formatting    |   16     16
Test Summary: | Pass  Total
Showoff       |   10     10
    Testing Showoff tests passed 

@timholy I'm creating a new PR due to some unresolved git issues I had with the previous commit. More details inside code review.

[isentropic]

This way users from 1.5 can also benefit

[isentropic]

The problem oftentimes arises due 0.0 present as usually 0.0 = 1e-55 (some weird power for floats). Then this would mean that if 0.0 is present, precision would go to 55. It is better to set that precision needed to display 0 should always be 0

[timholy]

julia> 1e-200 ≈ 0.0
false

I think you'll be better off keeping a min/max precision and capping the ratio between them.

[isentropic]

This logic is commented above and should be easy enough to follow. Though this function is less efficient (due to int parsing), I believe it is worth it as it greatly simplifies the logic

[isentropic]

Add printf as we should match the behavior to @sprintf as it was originally intended

[isentropic]

Compare first half (without exponent)

[isentropic]

Slightly breaking, as now engineering notation introduces the trailing 0, however it is now consistent with scientific as it also introduces the zero

[isentropic]

Regarding your comments from #30 (review), for Plots, the performance of Showoff is completely insignificant, as showoff is only called twice (thrice) per plot. The time to first plot comes from the backends, not Plots, and especially not from showoff. Merging this would greatly improve the user experience as it the issue #18 has been a major annoyance for a long time

[isentropic]

The test were modified in order to match @sprintf but that is not a major breakage I assume.
Perhaps gadfly maintainers could input their opinion about the performance hit?
@dcjones @bjarthur

[isentropic]

Also I noticed that Makie too, uses this @SimonDanisch.
Perhaps you could also comment about the performance?

[timholy]

Still not passing tests. Does it pass locally for you?

[timholy]

This seems close to mergeable, unless @SimonDanisch thinks the performance issues are serious. Certainly for Plots concerns about performance should not hold this back.

[timholy]

julia> 1e-200 ≈ 0.0
false

I think you'll be better off keeping a min/max precision and capping the ratio between them.

[isentropic]

It's strange, I pass the tests locally

[timholy]

Are you testing with Pkg.test? Sometimes that's different than include("runtests.jl"). Although perhaps the main thing to check is that you've pushed all your changes?

[isentropic]

@timholy What about now?
The probelm with 0.0 is that it has undefined rtol and it is hard to judge if a number is close to zero
The choice behind 1e-16 is somewhat arbitrary

[timholy]

My suggestion was to keep track of both the min and max of e10. Maybe something like this:

e10max = -(e10min = typemax(Int))
for y in ys
    _, e10 = Ryu.reduce_shortest(y)
    e10min = min(e10min, e10)
    e10max = max(e10max, e10)
end
precision = min(-e10min, -e10max+16)

That way if all the numbers are tiny, you won't truncate unnecessarily.

Not sure whether the old 0 was important for something, but this design would also allow you to incorporate that if necessary.

[isentropic]

With your code

function plain_precision_heuristic(xs::AbstractArray{<:AbstractFloat})
    ys = filter(isfinite, xs)
    e10max = -(e10min = typemax(Int))
    for y in ys
        _, e10 = Ryu.reduce_shortest(y)
        e10min = min(e10min, e10)
        e10max = max(e10max, e10)
        @show e10min, e10max
    end
    return precision = min(-e10min, -e10max+16)

end

The showoff fails, and I think it is due to 0.0s:

julia> showoff([0.0, 50000.0])
(e10min, e10max) = (-325, -325)
(e10min, e10max) = (-325, 0)
2-element Array{String,1}:
 "0"
 "5.00000000000000000×10⁴"

[isentropic]

So I still retain the zero number hack

function plain_precision_heuristic(xs::AbstractArray{<:AbstractFloat})
    ys = filter(isfinite, xs)
    e10max = -(e10min = typemax(Int))
    for y in ys
        if isapprox(y, 0, atol=1e-16)
            continue
        end
        _, e10 = Ryu.reduce_shortest(y)
        e10min = min(e10min, e10)
        e10max = max(e10max, e10)
        @show e10min, e10max
    end
    return precision = min(-e10min, -e10max+16)
end

and now the tests pass normally:

julia> showoff([0.0, 50000.0])
(e10min, e10max) = (0, 0)
2-element Array{String,1}:
 "0"
 "5.0×10⁴"

[isentropic]

More changes

[isentropic]

Regarding the tests, i pass them with both julia 1.5.3 and 1.4.2, of course the version of the julia needs to have Base.Ryu or else the tests would fail because of the fallback to Grisu.

~/Software/Showoff ryu-engineering2 ≡ 8s
❯ julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.5.3 (2020-11-09)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> include("test/runtests.jl")
Test Summary: | Pass  Total
Internals     |    7      7
Test Summary: | Pass  Total
Formatting    |   16     16
Test Summary: | Pass  Total
Showoff       |   10     10
Test.DefaultTestSet("Showoff", Any[], 10, false)

(@v1.5) pkg> test Showoff
    Testing Showoff
Status `/tmp/jl_2lmfLa/Project.toml`
  [42e2da0e] Grisu v1.0.0
  [992d4aef] Showoff v0.3.2 `~/Software/Showoff`
  [ade2ca70] Dates
  [de0858da] Printf
  [8dfed614] Test
Status `/tmp/jl_2lmfLa/Manifest.toml`
  [42e2da0e] Grisu v1.0.0
  [992d4aef] Showoff v0.3.2 `~/Software/Showoff`
  [2a0f44e3] Base64
  [ade2ca70] Dates
  [8ba89e20] Distributed
  [b77e0a4c] InteractiveUtils
  [56ddb016] Logging
  [d6f4376e] Markdown
  [de0858da] Printf
  [9a3f8284] Random
  [9e88b42a] Serialization
  [6462fe0b] Sockets
  [8dfed614] Test
  [4ec0a83e] Unicode
Test Summary: | Pass  Total
Internals     |    7      7
Test Summary: | Pass  Total
Formatting    |   16     16
Test Summary: | Pass  Total
Showoff       |   10     10
    Testing Showoff tests passed 

(@v1.5) pkg> 

[isentropic]

And thanks for looking into this issue @timholy and giving a code review, it is always helpful to have feedback 😄

[timholy]

Just take a look at my suggestion for handling the precision in the case of small values, and then once you decide how you want to handle that we can merge this.

Regarding the tests, once this merges I'll check locally and probably update the tests to run on GitHub Actions before merging to master. So don't worry about it unless you hear otherwise from me later.

[timholy]

My suggestion was to keep track of both the min and max of e10. Maybe something like this:

e10max = -(e10min = typemax(Int))
for y in ys
    _, e10 = Ryu.reduce_shortest(y)
    e10min = min(e10min, e10)
    e10max = max(e10max, e10)
end
precision = min(-e10min, -e10max+16)

That way if all the numbers are tiny, you won't truncate unnecessarily.

Not sure whether the old 0 was important for something, but this design would also allow you to incorporate that if necessary.

[isentropic]

handle 0s and min max exponents

[isentropic]

With your code

function plain_precision_heuristic(xs::AbstractArray{<:AbstractFloat})
    ys = filter(isfinite, xs)
    e10max = -(e10min = typemax(Int))
    for y in ys
        _, e10 = Ryu.reduce_shortest(y)
        e10min = min(e10min, e10)
        e10max = max(e10max, e10)
        @show e10min, e10max
    end
    return precision = min(-e10min, -e10max+16)

end

The showoff fails, and I think it is due to 0.0s:

julia> showoff([0.0, 50000.0])
(e10min, e10max) = (-325, -325)
(e10min, e10max) = (-325, 0)
2-element Array{String,1}:
 "0"
 "5.00000000000000000×10⁴"

[isentropic]

So I still retain the zero number hack

function plain_precision_heuristic(xs::AbstractArray{<:AbstractFloat})
    ys = filter(isfinite, xs)
    e10max = -(e10min = typemax(Int))
    for y in ys
        if isapprox(y, 0, atol=1e-16)
            continue
        end
        _, e10 = Ryu.reduce_shortest(y)
        e10min = min(e10min, e10)
        e10max = max(e10max, e10)
        @show e10min, e10max
    end
    return precision = min(-e10min, -e10max+16)
end

and now the tests pass normally:

julia> showoff([0.0, 50000.0])
(e10min, e10max) = (0, 0)
2-element Array{String,1}:
 "0"
 "5.0×10⁴"

[timholy]

That's a pretty ugly hack but let's go with it (I really don't use this package so...). I'll merge this into my branch and then see whether I can figure out the test failures. Thanks!

[timholy]

OK, so the problem is that the tests were failing on older versions of Julia, they were passing on 1.6 (which I presume is what you're using). Some of the things changed behaviors that look like bugs of the old spec; however, you also have some new bugs:

julia> Showoff.format_fixed_scientific(-10.0, 4, true)
"-1.0000×10⁰"

which we need to fix before we can merge this to master. (It should be -10.0000×10⁰.)

Finally, some changes you made to @testset "Showoff" only pertain to the Ryu code, not the Grisu code.

I've put more time into this than it warrants so I'm going to have to rely on you to make these fixes, then I'll wrap everything up and merge.