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

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

Merged
merged 6 commits into from Nov 3, 2017

Conversation

@kinson
Copy link
Contributor

commented Oct 22, 2017

See #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.

var runTimeNode = document.getElementById("raw-run-times");
var jobName = runTimeNode.getAttribute("data-job-name");
var minY = (statistics.minimum) - (0.1 * statistics.minimum);

This comment has been minimized.

Copy link
@devonestes

devonestes Oct 23, 2017

Collaborator

Wouldn't this logic be the same as statistics.minimum * 0.9? To me that seems like a clearer way to express what we're going for here.

This comment has been minimized.

Copy link
@kinson

kinson Oct 23, 2017

Author Contributor

Yes that is a much less confusing representation of the arithmetic and makes it much more readable

var inputHeadline = "<%= input_headline(input_name) %>"
drawRawRunTimeCharts(runTimes, inputHeadline);
drawRawRunTimeCharts(runTimes, inputHeadline, statistics);

This comment has been minimized.

Copy link
@devonestes

devonestes Oct 23, 2017

Collaborator

It looks like in your function definition you put the statistics as the second positional argument. I think having it as the third argument makes sense, but you'll need to update your function definition here.

This comment has been minimized.

Copy link
@kinson

kinson Oct 23, 2017

Author Contributor

Yeah I went back and forth on which to put first but I believe that the job_detail.html.eex reads better with the parameters ordered this way.

…etic for calculating minY
Copy link
Collaborator

left a comment

Ok, I think this might be ready to go! 🎉

I thought about what a test here might look like, but I'm not sure that the value that we'd get from it would outweigh the cost of having it. If this was somehow broken, I'm not sure it would be a huge problem for users, and to write a test for this that ensured that this behavior was working correctly I think we'd need to test the actual charts generated by plotly.js.

I thought about a test that checked to make sure there was a line in the generated HTML that defined minY correctly and that it was passed correctly to plotly.js, but it would be pretty easy for that test to pass and have things still be broken.

So I'm not going to merge this yet so there's time for @PragTob to weigh in on this. I'm not opposed to having that kind of test, but I would personally be ok without it.

@kinson

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2017

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

This comment has been minimized.

Copy link
Member

commented Oct 25, 2017

👋

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! 🎉

@PragTob

This comment has been minimized.

Copy link
Member

commented Oct 25, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2017

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:

  • The first thing I tried was to manually add a 0 value to the y axis. After looking at the y axis documentation for ticks I realized that if y0 is set, dtick must also be set to determine the spacing between ticks. This would result in having arbitrary (uniformly spaced) values along the y axis since the y0 is not necessarily a round number. Another way to use ticks to get a y0 label is to manually set each tick. This issue I see here is that this would require writing a robust function for calculating each tick based on the maximum and minimum y value that is well tested and handles any pair of min/max values and results in a well laid out graph. This is definitely doable but may be in the scope of another pr.
  • A second approach I have taken is trying to use an annotation. Annotations allow arbitrary one off notes about the data but I am having issues aligning the actual annotation with the other y axis labels. Right now I am using the following annotation and get an annotation that shifts the axis over. Were there to be a logical place to put this annotation that doesn't obfuscate the graph but still informs the user that it does not start at 0 that could be a good solution to this problem.
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

This comment has been minimized.

Copy link
Member

commented Oct 29, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2017

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

This comment has been minimized.

Copy link
Member

commented Nov 3, 2017

@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 🚀

@PragTob PragTob merged commit 6945852 into bencheeorg:master Nov 3, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-7.7%) to 92.308%
Details
@kinson

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.