bencheeorg / benchee_html

Draw pretty micro benchmarking charts in HTML and allow to export them as png for benchee
MIT License
58 stars 9 forks source link

Set Minimum Y Value on Raw Run Time Graphs to Reflect Actual Data Minimum #32

Closed kinson closed 7 years ago

kinson commented 7 years ago

See https://github.com/PragTob/benchee_html/issues/7

Hello @PragTob I have been looking through the plot.ly documentation and it appears that y0 is meant to be as an alternative to y -- which accepts the array of run time data -- and thus will not fit this solution. The next best option I have found is a range layout setting. Implementing this setting to satisfy the requirements of this issue requires using it's minimum and maximum values and then applying the arithmetic in the original issue post to get a revised minimum value for the y axis. I have done this in my pull request in the javascript assets.

kinson commented 7 years ago

Let me know if you want me to proceed with writing a test. If I were to do so would I be testing the generated html files (I guess actually the benchee.js file)?

PragTob commented 7 years ago

:wave:

The tests look at the generated HTML but it's hard to guess with the JS. We might want to/need to add js tests but that's obviously to big for this ticket :) So, no test should be good enough here. I also have a plan to add wallaby testing. Should put that into an issue.

Also apparently github is case sensitive so it needs to be @PragTob :D

I'll check this branch out and see how the results look like :)

Thanks a lot for digging into this! :tada:

PragTob commented 7 years ago

selection_046

Can we make the value of the "base" y axis show up in the bottom left? Or some sort of hint that we don't start at 0? Right now people could still mistake it. I mean, with the hovering it's easy to discover but if we just had another indicator for the base line I think it'd be clearer :)

kinson commented 7 years ago

I've been looking at a couple ways to do this and would love some feedback about which approach would be better suited for this feature:

annotations: [{ x: 0, y: minY, text: parseInt(minY), showarrow: false, xref: "x", yref: "y", xshift: -25 }]

annotation shifts axis

@PragTob @devonestes I would appreciate any feedback here! Thanks for your time reviewing this pr and answering my questions!

PragTob commented 7 years ago

@kinson the shown approach (I think it's the second one you mentioned) looks good enough for me :) The first indeed sounds to complicated imo. Thanks for taking all the time to research this and put time into this!

kinson commented 7 years ago

I just updated it to include the shown approach. Since this change displaces the y axis a little bit, should this something that could be disabled with a flag? Here is a screenshot again of the resulting graph with this most recent commit

screen shot 2017-11-03 at 1 39 00 pm
PragTob commented 7 years ago

@kinson thanks a ton! Phew... I know I have a lot of configuration options but I really don't want to have it :D So, honestly - until someone complains I'd keep it. And if someone complains we could look again if we could find another way to circumvent this problem. And only then would I add an option :)

So, I'm gonna hit merge :rocket:

kinson commented 7 years ago

@PragTob & @devonestes thanks for your guidance on this!