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

[RFC] Display histogram when showing trial #180

Closed
wants to merge 1 commit into from
Closed

Conversation

vchuravy
Copy link
Member

@vchuravy vchuravy commented Oct 17, 2020

I think we can all agree that there is no good choice for summary statistics, given the fact that benchmarks are often multi-modal. minimum can be a decent choice if one is focused on performance tuning a specific implementation, but if a user is comparing two different implementations it can be often misleading.

This PR adds a histogram to the output when showing a Trial, hopefully encouraging the user to think more deeply about comparing two results.

@vchuravy
Copy link
Member Author

As a reference https://tratt.net/laurie/blog/entries/minimum_times_tend_to_mislead_when_benchmarking.html gives some motivating examples.

@brenhinkeller
Copy link

This could be really fun to have! One main question might be: how much does adding the dependency on UnicodePlots increase the time for using BenchmarkTools? And if it's significant, is there a way to make a simple histogram without adding that dependency?

@brenhinkeller
Copy link

brenhinkeller commented Oct 18, 2020

It looks like the time for the UnicodePlots dependency could be significant relative to the current loading time

julia> @time using BenchmarkTools
  0.187841 seconds (245.64 k allocations: 15.819 MiB)
julia> @time using UnicodePlots
  0.553993 seconds (811.91 k allocations: 47.213 MiB)

but it also seems like it might not be hard just write a unicode histogram function from scratch, e.g.

function simpleunicodehistogram(x::AbstractArray; nbins::Integer=10, plotwidth::Integer=30, showcounts::Bool=true, xlabel="", ylabel="")
    # Find bounds, round them nicely
    l, u = extrema(x)
    digitsneeded = ceil(Int, -log10(u-l))+1
    l = floor(l, digits=digitsneeded)
    u = ceil(u, digits=digitsneeded)

    # Fill histogram
    dx = (u - l) / nbins
    histcounts = fill(0, nbins)
    @inbounds for i ∈ 1:length(x)
        index = ceil(Int, (x[i] - l) / dx)
        if 1 <= index <= nbins
            histcounts[index] += 1
        end
    end
    binedges = range(l,u,length=nbins+1)

    # Print the histogram
    blocks = [" ","▏","▎","▍","▌","▋","▊","▉","█","█"]
    scale = plotwidth/maximum(histcounts)
    lowerlabels = string.(round.(binedges[1:end-1], digits=digitsneeded+ceil(Int,log10(nbins)-1)))
    upperlabels = string.(round.(binedges[2:end], digits=digitsneeded+ceil(Int,log10(nbins)-1)))
    longestlower = maximum(length.(lowerlabels))
    longestupper = maximum(length.(upperlabels))
    println(ylabel*"\n")
    for i=1:nbins
        nblocks = histcounts[i] * scale
        blockstring = repeat("█", floor(Int, nblocks)) * blocks[ceil(Int,(nblocks - floor(nblocks))*8)+1]
        println(" (" * lowerlabels[i] * " "^(longestlower - length(lowerlabels[i])) *
                " - " * upperlabels[i] * " "^(longestupper - length(upperlabels[i])) *
                    "]  " * blockstring * (showcounts ? " $(histcounts[i])" : ""))
    end
    println("\n" * " "^max(plotwidth÷2 + 6 - length(xlabel)÷2, 0) * xlabel)
end
julia> simpleunicodehistogram(rand(10000)*100, ylabel="Time (ns)", xlabel="Number of observations", showcounts=true)
Time (ns)

 (0.0  - 10.0 ]  ████████████████████████████▋ 984
 (10.0 - 20.0 ]  █████████████████████████████▏ 1001
 (20.0 - 30.0 ]  █████████████████████████████▎ 1006
 (30.0 - 40.0 ]  ████████████████████████████▋ 985
 (40.0 - 50.0 ]  █████████████████████████████▎ 1007
 (50.0 - 60.0 ]  █████████████████████████████ 999
 (60.0 - 70.0 ]  ██████████████████████████████  1034
 (70.0 - 80.0 ]  ████████████████████████████▌ 982
 (80.0 - 90.0 ]  ████████████████████████████▋ 985
 (90.0 - 100.0]  █████████████████████████████▋ 1017

          Number of observations

@jrevels
Copy link
Member

jrevels commented Oct 18, 2020

Would it be better to just to have a front-and-center example in the docs of https://julialang.slack.com/archives/C681P8ABG/p1602495982012600?thread_ts=1602495855.012500&cid=C681P8ABG (screenshotted here for posterity)

Screen Shot 2020-10-18 at 2 05 05 PM

@jrevels
Copy link
Member

jrevels commented Oct 18, 2020

at @vchuravy's request, from Julia Slack (ref https://julialang.slack.com/archives/C681P8ABG/p1603023939039300)

Screen Shot 2020-10-18 at 2 06 33 PM

Screen Shot 2020-10-18 at 2 08 18 PM

@rfourquet
Copy link

Like @brenhinkeller I was also concerned with how this affects loading time. Loading BenchmarkTools is so fast that I have it in my startup.jl (I use it in most of my sessions), but I would hesitate to do that with UnicodePlots. Fortunately, it seems that both package are faster to load on 1.6, but adding UnicodePlots would still more than double the loading time:

julia> @time using BenchmarkTools # julia 1.6 
  0.085774 seconds (117.50 k allocations: 9.685 MiB, 48.95% compilation time)

julia> @time using UnicodePlots , BenchmarkTools # julia 1.6, in a new session
  0.224512 seconds (288.01 k allocations: 22.532 MiB, 19.69% compilation time)

julia> @time using BenchmarkTools # julia 1.5
 0.188177 seconds (234.56 k allocations: 14.028 MiB, 3.97% gc time)

julia> @time using UnicodePlots , BenchmarkTools # julia 1.5
  0.695784 seconds (761.20 k allocations: 45.126 MiB, 1.34% gc time)

@rfourquet
Copy link

Maybe Requires.jl could be used, or waiting for Pkg to implement "conditional dependencies" ?

@vchuravy
Copy link
Member Author

I think we can go with a custom implementation of a histogram, I was just to lazy to do that.

@ericphanson
Copy link
Contributor

I made a quick package to provide this: https://github.com/ericphanson/BenchmarkPlots.jl (no piracy, just re-using the @benchmark name for a new macro and re-exporting the rest).

ericphanson added a commit to ericphanson/BenchmarkHistograms.jl that referenced this pull request May 23, 2021
from JuliaCI/BenchmarkTools.jl#180 (comment)

Co-authored-by: C. Brenhin Keller <cbkeller@dartmouth.edu>
ericphanson added a commit to ericphanson/BenchmarkHistograms.jl that referenced this pull request May 23, 2021
from JuliaCI/BenchmarkTools.jl#180 (comment)

Co-authored-by: C. Brenhin Keller <cbkeller@dartmouth.edu>
ericphanson added a commit to ericphanson/BenchmarkHistograms.jl that referenced this pull request May 23, 2021
from JuliaCI/BenchmarkTools.jl#180 (comment)


Co-authored-by: C. Brenhin Keller <cbkeller@dartmouth.edu>
@tecosaur
Copy link
Collaborator

tecosaur commented Jun 1, 2021

For anyone subscribed to this, you'll likely find #217 of interest.

@vchuravy
Copy link
Member Author

vchuravy commented Jun 7, 2021

Closed in favour of #217

@vchuravy vchuravy closed this Jun 7, 2021
@vchuravy vchuravy deleted the vc/histograms branch June 7, 2021 09:26
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.

None yet

6 participants