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

Add nth percentile calculation #137

Merged
merged 16 commits into from Oct 12, 2017

Conversation

Projects
None yet
4 participants
@wasnotrice
Collaborator

wasnotrice commented Oct 10, 2017

Hi all,

Here's a first pass at adding nth percentile calculations for initial review (see #48). Essentially just:

  • a percentile function with tests (calculates any percentile value)
  • replacing the current median function with the new percentile function (since the median is the 50th percentile value)

There are lots of methods for calculating percentile, and I don't know much about the theory. Of the three most accepted, I chose the one that was explained in greatest detail. I left some resources in a comment for further reading, but I feel confident this method is suitable for our purposes, but it would be trivial to replace, or even offer alternate strategies.

This implementation has similar big-O complexity to the current median function (sorting and then traversing the list of samples), although it does remove one list traversal.

Questions:

  • Should percentile be included by default, or enabled with an option?

  • The issue suggests that we should only offer the 99th percentile. That seems like a reasonable default, although I suspect that folks will have widely varying ideas about what percentile value they are interested in, and it probably depends on their data. Do we want to offer more flexible configuration for percentile? There's nothing to do differently, calculation-wise, only figuring out a good user experience. Could also be added later.

Left to do:

  • add percentile value(s) to results
  • update output tests

wasnotrice added some commits Oct 10, 2017

@devonestes

This all looks pretty good to me! I just have a couple questions about the API.

I don't have much of that fancy book learning, so I would be interested to hear what @PragTob has to say about the algorithm choice.

@@ -159,7 +160,7 @@ defmodule Benchee.Statistics do
deviation = standard_deviation(run_times, average, iterations)
standard_dev_ratio = deviation / average
standard_dev_ips = ips * standard_dev_ratio
median = compute_median(run_times, iterations)
median = Percentile.percentile(run_times, iterations, 50)

This comment has been minimized.

@devonestes

devonestes Oct 10, 2017

Collaborator

Good call re-using this! I never would have put that together myself.

This comment has been minimized.

@wasnotrice

wasnotrice Oct 10, 2017

Collaborator

Hahaha, I think you would have. I didn't realize until I kept seeing the same values for median and 50th percentile. And reading!

This comment has been minimized.

@PragTob

PragTob Oct 10, 2017

Owner

it's non obvious yeah but this is how I imagined it. Thanks 😁

## Examples
iex> Benchee.Statistics.Percentile.percentile([5, 3, 4, 5, 1, 3, 1, 3], 8, 100)

This comment has been minimized.

@devonestes

devonestes Oct 10, 2017

Collaborator

Thank you for the lovely doctests 🎉

This comment has been minimized.

@PragTob

PragTob Oct 10, 2017

Owner

🎉 🎉 🎉

end
@spec percentile(list(number()), integer(), integer()) :: float()
def percentile(samples, number_of_samples, percentile_number) when percentile_number > 100 do

This comment has been minimized.

@devonestes

devonestes Oct 10, 2017

Collaborator

First off, do we want percentile/3 to be public? It seems to me like the caller can in all cases use percentile/2 and be totally cool.

What would you think about failing in this case and the one on line 40, and giving a helpful error message? I can't imagine that a user would give us a percentage of more than 99 or less than 1 on purpose. I think it would be likely that these would be cases of typos, and that by coercing these error cases we might be giving users results they think are strange and hiding their typos when we should be telling them how to fix their typos and get the results they want.

This comment has been minimized.

@wasnotrice

wasnotrice Oct 10, 2017

Collaborator

@devonestes so....the reason I have percentile/3 is because Statistics has both samples and iterations. I assumed that was to avoid calculating the length of the list over and over again (though I'm not certain about that). So—you're right, percentile/2 is really the only necessary interface, unless we want to be able to pass in number_of_samples (i.e. iterations) for efficiency. Have we.....benchmarked? 😁

This comment has been minimized.

@wasnotrice

wasnotrice Oct 10, 2017

Collaborator

What would you think about failing in this case and the one on line 40, and giving a helpful error message?

Actually that was my first inclination, but then I backed it out. What do you imagine, throwing something like raise ArgumentError, "bad percentile value: #{bad_value}. Percentile must be greater than 0 and less than 100"?

This comment has been minimized.

@PragTob

PragTob Oct 10, 2017

Owner

For performance see #139 - with what is reasonable to expect to hit us (~1 Million) it takes ~4ms on my machine, we might have multiple scenarios so let's say * 10 that gives 40ms which is ok I guess ;) For 10 Million we'd be at about 300ms total according to this calculation. Non noticable for the given scenario user wise.
However, that's not the real reason I reused iterations - it just feels bad to me when I have calculated a value in a top level method and then just recalculate the value in a method directly called from that method. Such waste, could just pass it along.
When I eventually extract a library called statistex from benchee I think I'd keep the /3 version because I'd like to give that possibility to people as they might operate on bigger data and it might make a different for them.

Raising an argument error sounds nice! 👍

This comment has been minimized.

@wasnotrice

wasnotrice Oct 10, 2017

Collaborator

so, not sure where we stand with percentile/2 vs percentile/3. Speed-wise, not a big deal (thanks @PragTob). Actually, percentile/2 is not used here at all, I just included it because it felt like the best interface. And I think there is a difference between carrying along the precomputed length within a module, versus allowing it across modules. I mean, if you pass the wrong number there, you get wrong results. So, for that reason, I'm going to remove the percentile/3 version. It's trivial to add back again in the future.

percentile_value(sorted, rank)
end
defp percentile_value(sorted, rank) when trunc(rank) == 0 do

This comment has been minimized.

@devonestes

devonestes Oct 10, 2017

Collaborator

I had never seen trunc before - very cool!

PragTob added a commit that referenced this pull request Oct 10, 2017

@PragTob

Helllooooo 💚

This looks great overall, thanks so much! Comments are mostly minor and chiming in on how we want to behave in certain cases and some background (aka what was Tobi thinking?!?!?!) :)

yum yum yummy nth percentile!

img_20170723_143731

@@ -159,7 +160,7 @@ defmodule Benchee.Statistics do
deviation = standard_deviation(run_times, average, iterations)
standard_dev_ratio = deviation / average
standard_dev_ips = ips * standard_dev_ratio
median = compute_median(run_times, iterations)
median = Percentile.percentile(run_times, iterations, 50)

This comment has been minimized.

@PragTob

PragTob Oct 10, 2017

Owner

it's non obvious yeah but this is how I imagined it. Thanks 😁

Calculates the value at the `percentile_number`-th percentile. Think of this as the
value below which `percentile_number` percent of the samples lie. For example,
if `Benchee.Statistics.Percentile.percentile(samples, 99)` == 123.45,
99% of samples are less than 123.45.

This comment has been minimized.

@PragTob

PragTob Oct 10, 2017

Owner

less than or equal to imo?

This comment has been minimized.

@wasnotrice

wasnotrice Oct 10, 2017

Collaborator

Definitely open to improvements in the documentation. My understanding is that this value we are calculating is a "less than" number. Here's what Wikipedia says:

A percentile (or a centile) is a measure used in statistics indicating the value below which a given percentage of observations in a group of observations fall. For example, the 20th percentile is the value (or score) below which 20% of the observations may be found.

The term percentile and the related term percentile rank are often used in the reporting of scores from norm-referenced tests. For example, if a score is at the 86th percentile, where 86 is the percentile rank, it is equal to the value below which 86% of the observations may be found (carefully contrast with in the 86th percentile, which means the score is at or below the value of which 86% of the observations may be found - every score is in the 100th percentile).

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

imo it's or equal. E.g.

When we have 10 values 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 - the 80th percentile would be 8 and 8 is a member of that 80th percentile group so from my understanding it should be less than or equal

This comment has been minimized.

@wasnotrice

wasnotrice Oct 11, 2017

Collaborator

However....

iex(2)> Percentile.percentile([1, 2, 3, 4, 5, 6, 7, 8, 9, 10], 80)
8.8

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

but.... but... I guess I have to take a look at the calculation again then and the involved values or the algorithm used to obtain them. I had some suspicions about indexes being off somewhere... for my understanding of 80th percentile (aka 80 percent of the data) this should most definitely return 8 - or where am I going wrong?

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

After cross checking this seems to be the right value (tm) but it still appearsto be weird to me. I mean 80% of my values are up until including 8. In this particular case I would have understood if it's like 8.2 or something... 8.8 just plain seems too weird. Need to up my percentile knowledge apparently

## Examples
iex> Benchee.Statistics.Percentile.percentile([5, 3, 4, 5, 1, 3, 1, 3], 8, 100)

This comment has been minimized.

@PragTob

PragTob Oct 10, 2017

Owner

🎉 🎉 🎉

end
@spec percentile(list(number()), integer(), integer()) :: float()
def percentile(samples, number_of_samples, percentile_number) when percentile_number > 100 do

This comment has been minimized.

@PragTob

PragTob Oct 10, 2017

Owner

For performance see #139 - with what is reasonable to expect to hit us (~1 Million) it takes ~4ms on my machine, we might have multiple scenarios so let's say * 10 that gives 40ms which is ok I guess ;) For 10 Million we'd be at about 300ms total according to this calculation. Non noticable for the given scenario user wise.
However, that's not the real reason I reused iterations - it just feels bad to me when I have calculated a value in a top level method and then just recalculate the value in a method directly called from that method. Such waste, could just pass it along.
When I eventually extract a library called statistex from benchee I think I'd keep the /3 version because I'd like to give that possibility to people as they might operate on bigger data and it might make a different for them.

Raising an argument error sounds nice! 👍

def percentile(samples, number_of_samples, percentile_number) do
sorted = Enum.sort(samples)
rank = (percentile_number / 100) * max(0, number_of_samples + 1)

This comment has been minimized.

@PragTob

PragTob Oct 10, 2017

Owner

why number_of_samples + 1 - i.e. why the + 1, shouldn't we be good without it?

This comment has been minimized.

@wasnotrice

wasnotrice Oct 10, 2017

Collaborator

@PragTob I had the same thought. But it's part of the formula. everything comes out wrong if you just use number_of_samples. Imagine you have a list of 10 items, and you're looking for the 50th percentile (median):

currently: rank = (50 / 100) * 11 = 5.5
without + 1: rank = (50 / 100) * 10 = 5

So, as-is, we look for the value halfway between the 5th and 6th elements (yay, median). If we remove the +1, then we look for the value of the 5th element (boo, not the median).

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

makes sense... for the odd case of 9, 1/2 * 9 would also be 4.5 but we are looking for 5. Thanks.

However.... dum dum dum - we are 0 indexed. So in my example above we should be looking at number 4 not number 5? But maybe that is redeemed through the drop... weird.

I think it's ok as the median tests I wrote back in the day + your tests are passing. One of these things where I trust the tests more than my brain :D

#
# For more information on interpolation strategies, see:
# - https://stat.ethz.ch/R-manual/R-devel/library/stats/html/quantile.html
# - http://www.itl.nist.gov/div898/handbook/prc/section2/prc262.htm

This comment has been minimized.

@PragTob

PragTob Oct 10, 2017

Owner

👍 information

# - http://www.itl.nist.gov/div898/handbook/prc/section2/prc262.htm
defp interpolation_value(lower_bound, upper_bound, rank) do
interpolation_weight = rank - trunc(rank)
interpolation_weight * (upper_bound - lower_bound)

This comment has been minimized.

@PragTob
defp percentile_value(sorted, rank) do
index = trunc(rank)
[lower_bound, upper_bound | _] = Enum.drop(sorted, index - 1)

This comment has been minimized.

@PragTob

PragTob Oct 10, 2017

Owner

at first I was pretty skeptical, but this seems like a great usage of drop - thanks! 👍

95.1682
]
@tag run: true

This comment has been minimized.

@PragTob

PragTob Oct 10, 2017

Owner

we don't need the tag, do we? :)

This comment has been minimized.

@wasnotrice

wasnotrice Oct 10, 2017

Collaborator

😊

@PragTob

This comment has been minimized.

Owner

PragTob commented Oct 10, 2017

Ah, forgot to answer questions.

Re when to display:
I like the idea of always displaying 99th percentile alongside median if the output doesn't get too big. Could totally be done in a separate PR though imo. This is good enough to get in imo (yay small PRs!). Otherwise it'd be part of #50

Re custom percentiles:
Definitely a cool possible future feature I wanted to have. It's a whole other beast though.

  • Do we allow people to override our value? So instead of 99th percentile they can go: nth_percentile: 95 - might be good and easy enough
  • Can the user specify multiple ones aka nth_percentile: [25, 50, 75, 95] ? Might be useful, but display gets harder and might play into #50 - totally different beast and can be done later imo :)

wasnotrice added some commits Oct 11, 2017

Remove percentile/3, raise on bad percentile
When measured, counting the length of the list was fast enough.
Add percentiles to statistics
Also adds the ability to pass a list of percentile ranks into
`percentile/2` to calculate multiple percentiles without sorting
the list multiple times.

This form is used currently in `Statistics`, so we only sort the list
once and then calculate the 50th and 99th percentiles (where 50th is the
median).
@wasnotrice

This comment has been minimized.

Collaborator

wasnotrice commented Oct 11, 2017

@PragTob @devonestes thanks for the review! I've incorporated most of your feedback in this latest commit. As I worked through it a bit, I ended up adding back percentile/3 as a private function, and saving off the number of samples internally, so that we can calculate multiple percentiles from the same already-sorted list of samples. This already comes in handy so that we don't do a separate sort for median, and lays the foundation for adding other percentiles in the future if need be.

What do you think about using a map to return multiple percentile values?

%{50 => 45.67, 99 => 67.89}

I also considered a list of tuples:

[{50, 45.67}, {99, 67.89}]

but the map seemed nicer for reading out a particular percentile, like the median. Wanted to get your feedback before I got too much deeper into teasing out the test expectations.

There are some weird long line breaks (and even a short variable name) in there to pacify Credo 🔨

Also, you'll notice I added the percentile header to the console output, but I ran into this map question before I got the data in there

@PragTob

This comment has been minimized.

Owner

PragTob commented Oct 11, 2017

@wasnotrice definitely as a map 👍

Might do review of the other additions later, wifi is broken here 💢 don't wanna stress my mobile Internet too much :)

@PragTob

Got around to the review, mostly minor + questions :)

|> :io_lib.format([-label_width, "Name", @ips_width, "ips",
@average_width, "average",
@deviation_width, "deviation", @median_width, "median"])
@deviation_width, "deviation", @median_width, "median",
@percentile_width, "P99"])

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

him P99 seems weird to me, 99th or so might be better or wdyt?

This comment has been minimized.

@wasnotrice

wasnotrice Oct 11, 2017

Collaborator

I went back and forth on this: 99th percentile is too long. I chose P99 because it's actually a thing people use (according to the internet). But they are probably professional statisticians. I like 99th — we just have to be sure to handle 1st, 2nd, and 3rd too

Other options:

  • 99th
  • 99%
  • 99th %
  • 99th %ile

I'll extract it to a function so we can change it easily

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

99% looks even better to me 👍

This comment has been minimized.

@devonestes

devonestes Oct 11, 2017

Collaborator

Maybe 99th %? I'll keep thinking of other possibilities, too, but that's the only one that jumped out to me.

typical value you see.
* percentiles - a map of percentile ranks. These are the values below
which x% of the run times lie. For example, 99% of run times are shorter
than the 99th percentile (P99) rank.

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

shorter or equal to :)

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

and apparently I stand corrected

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

apparently I stand corrected

@@ -160,7 +167,8 @@ defmodule Benchee.Statistics do
deviation = standard_deviation(run_times, average, iterations)
standard_dev_ratio = deviation / average
standard_dev_ips = ips * standard_dev_ratio
median = Percentile.percentile(run_times, iterations, 50)
percentiles = Percentile.percentile(run_times, [50, 99])
median = Map.get(percentiles, 50)

This comment has been minimized.

@PragTob

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

I prefer Map.fetch! though as I like to blow up early :)

This comment has been minimized.

@wasnotrice

wasnotrice Oct 11, 2017

Collaborator

👍

Calculates the value at the `percentile_number`-th percentile. Think of this as the
value below which `percentile_number` percent of the samples lie. For example,
if `Benchee.Statistics.Percentile.percentile(samples, 99)` == 123.45,
99% of samples are less than 123.45.

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

imo it's or equal. E.g.

When we have 10 values 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 - the 80th percentile would be 8 and 8 is a member of that 80th percentile group so from my understanding it should be less than or equal

5.0
iex> Benchee.Statistics.Percentile.percentile([5, 3, 4, 5, 1, 3, 1, 3], [50, 75, 99])
%{50 => 3.0, 75 => 4.75, 99 => 5.0}

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

I feel a bit uneasy about having a different return type for one argument versus list arguments. Not saying needs to change just expressing fears. I think from a user's perspective this here is usually nicer but can come unexpected

This comment has been minimized.

@wasnotrice

wasnotrice Oct 11, 2017

Collaborator

Good point. What do you think about renaming this function percentiles/2, always passing in a list of percentiles, and always returning a map. I kind of like that idea.

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

Sounds good to me 👍

@spec percentile(list(number()), integer()) :: float()
def percentile(samples, percentile_number) do
percentile(samples, length(samples), percentile_number)
@spec percentile(list(number()), number() | list(number())) ::

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

do we need the parentheses? Looks a bit... distracting.

This comment has been minimized.

@devonestes

devonestes Oct 11, 2017

Collaborator

We're getting them with the formatter whether we like it or not, so I'd say we keep them. We could use the [number()] syntax for the lists to remove a couple parentheses.

This comment has been minimized.

@wasnotrice

wasnotrice Oct 11, 2017

Collaborator

I only added those because I noticed in my elixir code formatter pull requests, the code formatter was doing that. Or I thought so. @devonestes you wrote an article—does the code formatter add parentheses to types? :)

This comment has been minimized.

@wasnotrice

wasnotrice Oct 11, 2017

Collaborator

@devonestes thanks for answering my question......before I asked it!

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

I'm already starting to "like" the formatter... ;P

(seriously though, why do types in type specs have to look like function calls... makes little sense to me, maybe Jose has a rationale somewhere (I'm sure he has, question is just if he wrote it down somewhere))

|> Enum.reduce(%{}, fn percentile_rank, acc ->
perc = percentile(sorted_samples, number_of_samples, percentile_rank)
Map.put(acc, percentile_rank, perc)
end)

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

This is just one pipe, so I think it can be written a bit more easily as Enum.reduce(percentile_ranks, %{} ...

This comment has been minimized.

@wasnotrice

wasnotrice Oct 11, 2017

Collaborator

CREDO!!! Fails on the line length check. Can't get that one thru CI

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

Huh, I see you like credo :D I might come around on not enforcing 80 char width but not quite yet :D

Worst case I'd break the arguments to multiple lines.

This comment has been minimized.

@devonestes

devonestes Oct 11, 2017

Collaborator

It's going to be 98 characters with the formatter. That's one of the few things I disagree with, but c'est la vie! I'd be ok with changing Credo's line length check to be 98 characters now, since we know that's in the future anyway.

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

98? 98????? Not 100. No 98? exahles slowly

(for people who don't know me, I'm mostly just joking around here but am genuinely confused)

Go ahead @devonestes

This comment has been minimized.

@wasnotrice

wasnotrice Oct 11, 2017

Collaborator

I actually really like credo. But the hard 80-char limit is really tough in certain cases, like when you are trying to put a url in a comment

This comment has been minimized.

@OvermindDL1

OvermindDL1 Oct 11, 2017

/me always uses a 120 char limit everywhere for everything, so this 98 limit in the new formatter is blegh ^.^;

sorted = Enum.sort(samples)
rank = (percentile_number / 100) * max(0, number_of_samples + 1)
percentile_value(sorted, rank)
defp percentile(sorted_samples, number_of_samples, percentile_rank) do

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

these days we usually group the same function together without a newline inbetween, should find/create a credo rule for that :)

This comment has been minimized.

@wasnotrice

wasnotrice Oct 11, 2017

Collaborator

@devonestes code formatter now separates these functions, no?

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

what... everything I know is a lie then I believe I started adopting that style after seeing how it's done in elixir core :| I probably should play with the formatter a little...

This comment has been minimized.

@devonestes

devonestes Oct 11, 2017

Collaborator

Yep, there's always a space between functions defined with the do-newline-end syntax. If you want no line between functions, you can use , do: in the pure AST form.

This comment has been minimized.

@PragTob

This comment has been minimized.

@wasnotrice

wasnotrice Oct 11, 2017

Collaborator

@PragTob yes, some of the code formatter styles are sad, I agree.

percentile_value(sorted, rank)
defp percentile(sorted_samples, number_of_samples, percentile_rank) do
rank = (percentile_rank / 100) * max(0, number_of_samples + 1)
percentile_value(sorted_samples, rank)
end
defp percentile_value(sorted, rank) when trunc(rank) == 0 do

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

With our new error throwing ways we shouldn't need this method anymore :)

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

Same thing for the function version below but it won't let me comment :)

This comment has been minimized.

@wasnotrice

wasnotrice Oct 11, 2017

Collaborator

this probably got buried in my initial comment, but I added this back as a private function mainly so we don't have to sort the list again while calculating multiple percentiles on the same list

This comment has been minimized.

@PragTob

PragTob Oct 11, 2017

Owner

i meant the specific == 0 but maybe we can't get rid off it :'(

This comment has been minimized.

@wasnotrice

wasnotrice Oct 11, 2017

Collaborator

I'll try—it's actually a guard for the Enum.drop later, to make sure we have enough elements to make a match. But I'm not sure it adequately protects us. I need to write some more tests.

@@ -18,7 +18,8 @@ defmodule Benchee.Statistics.PercentileTest do
95.1682
]
@tag run: true
# Test data from:
# http://www.itl.nist.gov/div898/handbook/prc/section2/prc262.htm

This comment has been minimized.

@PragTob

wasnotrice added some commits Oct 11, 2017

Add tests for small lists, raise on empty list
Empty lists cause errors calculating other statistics, too, like min and
max
Format test scenarios consistently
Adding percentile made it impossible to put all statistics on one line.
Reformatting makes it easier to add new items as they come along.
@wasnotrice

This comment has been minimized.

Collaborator

wasnotrice commented Oct 11, 2017

I think this is ready to merge. I probably should have put the console formatter stuff in another commit, since we may want it to be optional, per #50. But I can take that one up next, since I've just been in that code.

@devonestes

Looks good to me 🎉 🎉 I'll wait for @PragTob to take a peek before merging in case he spots something I missed.

@PragTob

Basically one minor question about function length otherwise 👌 💃 🎉 💚

percentile_ranks
|> List.wrap

This comment has been minimized.

@PragTob

PragTob Oct 12, 2017

Owner

Had to look this one up, nice! :)

|> Enum.reverse
|> hd
|> to_float
end

This comment has been minimized.

@PragTob

PragTob Oct 12, 2017

Owner

Method is a bit too long for my sensibilities :D I feel like at least the last case and maybe the first one could go into a separate function (no huge gains for the first). Wdyt?

(not totally opposed to shipping as is)

median: 195.5,
percentiles: %{99 => 300.1}
}
}

This comment has been minimized.

@PragTob

PragTob Oct 12, 2017

Owner

Nice formatting 👍

assert Float.round(result, 4) == 95.1981
end
test "an empty list raises an argument error" do
assert_raise ArgumentError, fn -> Percentile.percentiles([], [1]) end

This comment has been minimized.

@PragTob

PragTob Oct 12, 2017

Owner

👍 good edge case testing!

%{99 => result} = Percentile.percentiles(samples, [99])
assert result == 300.0
end
end

This comment has been minimized.

@PragTob

PragTob Oct 12, 2017

Owner

even better edge case testing! :)

@PragTob

This comment has been minimized.

Owner

PragTob commented Oct 12, 2017

@wasnotrice ah forgot to say it, I think we wanna display the default percentile all the time so it should be alright, only for optional more percentiles they should land in the additional ones (or so I think). Depends on how wide the output is now :)

Once we have this on master, some playing around might also be nice. I.e. 99th percentile might be a sensible default, but it might not be. Iirc Skylight uses 95th so maybe that ends up being the better default, we might wanna try and play with that a bit.

@wasnotrice

This comment has been minimized.

Collaborator

wasnotrice commented Oct 12, 2017

@PragTob good call on splitting that case up. I ended up splitting the whole thing into a set of function clauses, which makes the flow clearer, I think.

I agree that 99th percentile might not be the best default. It probably depends a lot on the data. I envision it being easy to configure a single percentile to show in the main results, and maybe a set of percentiles to display in the extended results.

The current display with 99th % for examples/simple_measure.exs is 75 characters:

Benchmarking map...

Name           ips        average  deviation         median         99th %
map        20.54 K       48.68 μs    ±44.07%          43 μs         109 μs

Thanks for the great review @PragTob @devonestes 🎉

@PragTob

This comment has been minimized.

Owner

PragTob commented Oct 12, 2017

So with all this, this can go in 💚 🎉

Yeah, depending on what we want 99th percentile doesn't seem like a great default. I mean right now it seems to be more or less a worst case performance but we already have this with the maximum metric (we just don't display it... yet ;P). I envisioned it more as a "worst case without the outliers" sort of thing.

As for configuration, I think it'd be fair to say that the first value in the percentiles list always gets displayed. But that's for another issue.

Thanks so much @wasnotrice !

@PragTob PragTob merged commit cc88093 into master Oct 12, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.3%) to 94.086%
Details

@PragTob PragTob deleted the add-percentile branch Oct 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment