Add unit scaling to Console formatter #29
Conversation
Existing duration labels had spaces between the value and the label. This commit adopts that format for all of the labels.
Specifies this function in the Unit behaviour
| @@ -5,8 +5,7 @@ defmodule Benchee.Formatters.Console do | |||
| """ | |||
|
|
|||
| alias Benchee.Statistics | |||
|
|
|||
| import Benchee.Unit, only: [float_precision: 1] | |||
| alias Benchee.Unit.{Count, Duration} | |||
| # TODO: Simplify when dropping support for 1.2 | ||
| # For compatibility with Elixir 1.2. In 1.3, the following group-reduce-map | ||
| # can b replaced by a single call to `group_by/3` | ||
| # Enum.group_by(fn({measurement, _}) -> measurement end, fn({_, value}) -> value end) |
There was a problem hiding this comment.
Could you please open a ticket for this? Like Drop Elixir 1.2 support and then we could have a check list what to change? Oh and mention that we'd probably do this with an Elixir 1.4 release :)
| # Produces a map like | ||
| # %{run_time: [12345, 15431, 13222], ips: [1, 2, 3]} | ||
| collected_values = jobs | ||
| |> Enum.flat_map(fn({_name, job}) -> Map.to_list(job) end) |
There was a problem hiding this comment.
hm code formatting wise I'm usually a fan (but not yet decided on which one) of:
mine = value
|> pipe_stuff
# or
mine =
value
|> pipe_stuffmakes the start of the pipe more visible I think and more distinguished. What do you think?
There was a problem hiding this comment.
agree. went with option #2 since the pipes are long
| # For compatibility with Elixir 1.2. In 1.3, the following group-reduce-map | ||
| # can b replaced by a single call to `group_by/3` | ||
| # Enum.group_by(fn({measurement, _}) -> measurement end, fn({_, value}) -> value end) | ||
| |> Enum.group_by(fn({measurement, _unit}) -> measurement end) |
There was a problem hiding this comment.
is the second parameter here really the unit? Isn't it the value? e.g. shouldn't this be something like {"average", 45454}.
If I'm correct, then measurement also seems not totally spot on to me. Maybe "statistic"/"statistic_name"/"stat" - but apprently measurement is exactly the correct word. Weird I always thought of measurement as something that I measure directly e.g. a run time and not a derived computed value but Wikipedia calls it like this:
The median is a commonly used measure of the properties of a data set in statistics and probability theory.
There was a problem hiding this comment.
I used "stat_name" since Statistic is already a concept
| # Enum.group_by(fn({measurement, _}) -> measurement end, fn({_, value}) -> value end) | ||
| |> Enum.group_by(fn({measurement, _unit}) -> measurement end) | ||
| |> Enum.reduce(%{}, fn({measurement, occurrences}, acc) -> | ||
| Map.put(acc, measurement, Enum.map(occurrences, fn({_meas, value}) -> value end)) |
| end | ||
|
|
||
| defp run_time_out(average, unit) do | ||
| Duration.format(Duration.scale(average, unit)) |
There was a problem hiding this comment.
😍 how that nitty gritty format code now just lives somewhere else more focussed and contained :)
| {1.234, :millisecond} | ||
|
|
||
| iex(8)> Benchee.Unit.Duration.scale(11_234_567_890.123) | ||
| iex> Benchee.Unit.Duration.scale(11_234_567_890.123) |
| {12.345, :millisecond} | ||
|
|
||
| iex(5)> Benchee.Unit.Duration.scale(12345, :minute) | ||
| {2.0575e-4, :minute} |
| assert result =~ "13000" | ||
| assert result =~ "14000" | ||
| assert result =~ "140.00" |
There was a problem hiding this comment.
hm why is the unit of the median not checked? Should be there? :)
| A separator that should appear between a value and a unit label. For example, | ||
| a space: `5.67 M` or an empty string: `5.67M` | ||
| """ | ||
| @callback separator :: String.t |
There was a problem hiding this comment.
The separator always seems to be " " atm. What's the rationale behind making it dependent on the unit?
There was a problem hiding this comment.
btw. I'm open to changing it back to be directly attached to the number... for some reason I thought a space was more common.. but they also do it inconsistently e.g. i/s has a space while M/K don't. I also feel like M/K don't need one but Million would need to have one. Weird, huh? :)
There was a problem hiding this comment.
You're right, it's always " " right now. Mostly for the reasons that you are thinking about, too.
Initally, I was imagining that counts would be formatted like in du: 100K, with no space. But then I noticed that times were already formatted with a space. And I also thought about the long unit labels, and how you might want 100K but you would probably never want 100Thousand. So it seemed like something that
- might come down to personal preference of the user
- might need to be different for time vs count units
So, the idea is to lay the groundwork for allowing users to configure the separator as they desire (would come in #28). See Common.format/3 (currently not exposed on the Unit modules)
|
Thanks a lot 🎉 I'll write the Changelog once I get the time :) Or you can alsso take a stab at it if you're faster than me :) |
|
Awesome, thanks! I updated the CHANGELOG in 91ecf2b |
Integrates unit scaling into the Console formatter (solution for #27)
Features:
MforMillion10 μsLimitations: