Allow for customizable percentiles in extended statistics #230
Conversation
There was a problem hiding this comment.
Thanks a lot and sorry for taking such a long time 🙏
I think we should put the configuration outside of the formatter.
First, right now the statistics code looks at formatter configuration (later layer) which I don't think is good + other formatters will want to display percentiles as well and therefore having it more global might be good.
I think nesting it under something like statistics makes sense so that we already had a place to put future statistics configuration - although that might never come so it might be premature.
(I was thinking about a feature to benchmark until a degree of confidence is reached not sure I'd put that under statistics but might be good)
lib/benchee/configuration.ex
Outdated
| @@ -124,6 +125,9 @@ defmodule Benchee.Configuration do | |||
| (x times slower than) is shown (true/false). Enabled by default. | |||
| * `extended_statistics` - display more statistics, aka `minimum`, | |||
| `maximum`, `sample_size` and `mode`. Disabled by default. | |||
| * `percentiles` - if you are using extended statistics and want to see the | |||
| results for certain percentiles of results beyond just the median. | |||
| Defaults to [50, 99] to calculate the 50th and 99th percentiles. | |||
There was a problem hiding this comment.
I think that's the wrong place to put it (sorry I had a lot going on so that this comment comes so late).
I think this should either be a top level configuration option or nested under statistics - the console formatter is wrong imo as it affects statistics and might be grabbed by multiple formatters.
| } | ||
| } | ||
|
|
||
| assert ^expected = result | ||
| assert expected == result |
lib/benchee/statistics.ex
Outdated
|
|
||
| percentiles = | ||
| if is_nil(config) do | ||
| [50, 99] |
There was a problem hiding this comment.
we shouldn't need this with the appropriate default in the configuration right? I guess this was added to make testing less troublesome? I can support that but wonder if we could handle it differently 🤔
There was a problem hiding this comment.
yeah, we had some tests that didn't have a config, which was failing the tests. I've just looked at it again, and it's only 7 tests, so I might as well just update them so we can remove this check.
Before these were always set to 50 and 99, but now users can customize the percentiles they want to see in extended statistics. We always need the 50th percentile to get the median, so even if a user doesn't set that, it will be calculated anyway.
Before these were always set to 50 and 99, but now users can customize
the percentiles they want to see in extended statistics. We always need
the 50th percentile to get the median, so even if a user doesn't set
that, it will be calculated anyway.
Resolves #151