Port HTML report to chart.js #229
Conversation
|
Hooray! It looks like this is still a draft, so let me know when this is ready for a full review. |
jonascarpay
force-pushed
the
master
branch
4 times, most recently
from
b65e7db
to
f6638ca
Compare
|
This is ready for review. The fact that it's one commit is a bit unfortunate, but since most work happened in large files that have since disappeared there is not really a nice way to make it more atomic. |
|
Awesome! I'll do a more thorough review of this patch over the weekend, but a quick before-and-after comparison makes me notice two things right off the bat:
Some other random thoughts:
|
Yeah, zooming was left out intentionally. I figured the primary use case was to see bars that got dwarfed by others, which I felt the click-to-focus was a better interface for. It's possible to do zooming, but it also requires a disproportionate amount of code to get right. What do you think?
Makes sense, I added a note.
Both lexical and colex sort by group, with lexical the head is most significant and with colex the tail is most significant (i.e. big-endian vs little-endian). I suppose "by group" would be a better name for lexical, do you know a good, self-evident name for colex?
414911b expanded the radius, do you think it's OK now? |
|
@RyanGlScott Note that |
considerate
force-pushed
the
master
branch
2 times, most recently
from
91886d7
to
2eec914
Compare
|
@jonascarpay @RyanGlScott I added titles to the kernel density estimates and scatter plots to provide the same amount of context as the previous version for those plots. I think it might somewhat redundant and would personally like to keep the output as free from noise as possible. What do you think? |
There was a problem hiding this comment.
Thanks for looking into the legend issue. I'm now reminded of the existence of #207, however, where people specifically complained that legends were making their charts unreadable. In light of this, I'm wondering if I should take back my previous suggestion to reinstate the titles. Or, at the very least, allow one to toggle whether the legend appears or not with a mouse click. What do you think?
Yeah, zooming was left out intentionally. I figured the primary use case was to see bars that got dwarfed by others, which I felt the click-to-focus was a better interface for. It's possible to do zooming, but it also requires a disproportionate amount of code to get right. What do you think?
Upon further thought, I agree with your assessment that click-to-zoom is a better interface for this.
Both lexical and colex sort by group, with lexical the head is most significant and with colex the tail is most significant (i.e. big-endian vs little-endian). I suppose "by group" would be a better name for lexical, do you know a good, self-evident name for colex?
Ah, "colex" stands for "colexical"! That's illuminating, especially since I couldn't even find what colex meant in a search engine. At the very least, I would recommend expanding "colex" to "colexical". I would also recommend spelling out what each of these options mean in the "want to understand this report?" section.
414911b expanded the radius, do you think it's OK now?
Much better, thanks!
I added titles to the kernel density estimates and scatter plots to provide the same amount of context as the previous version for those plots.
I think it might somewhat redundant and would personally like to keep the output as free from noise as possible. What do you think?
Personally, I'm a fan of the titles and labels, since it makes the information more self-contained. On the other hand, the y-axis labels do cause the actual charts themselves to shrink a fair bit in order to accommodate the new text, so I can definitely understand your reservations there. Ultimately, I think this comes down to a judgment call about optimizing the amount of information displayed versus the size of the chart. Given that the axis labels are implied by the titles themselves, I think I would be fine with omitting them.
|
I've timed out for now, but this is looking very promising. More review to come later! |
|
Is there a preview link that works on mobile? :)) |
|
Alright, that should do it for the feedback up to now. @cartazio Refined Github gives me this preview link, I think that should work. Most new features work best on reports with a bigger spread of orders of magnitude though, but it'll do. Also, it does look like on mobile part of the overview chart gets eaten, will look into that. |
|
Hrmm. One good thing to test is having an example plot where there’s a huge
range of magnitudes of values.
Def like how this one looks so far.
One issue with the previous was some sort of huge algorithmic penalty for
actually rendering some of the plots. Was that part of the issues with the
old flot code?
…On Fri, Nov 13, 2020 at 9:16 AM Jonas Carpay ***@***.***> wrote:
Alright, that should do it for the feedback up to now.
@cartazio <https://github.com/cartazio> Refined Github gives me this
<https://refined-github-html-preview.kidonng.workers.dev/jonascarpay/criterion/raw/master/www/report.html>
preview link, I think that should work. It does look like on mobile part of
the overview chart gets eaten, will look into that.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#229 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAABBQWG7D5GEHPXBYTXJRLSPVEUJANCNFSM4TNDRVBQ>
.
|
This is the benchmark suite we've used for that purpose before
Hmm, I don't remember there being any such algorithm in the old code. Do you remember how that issue would manifest?
No, the issue was a choice between a) the stable version that didn't support things like log axes and b) the new version that was unstable in every sense of the word |
Without running any proper benchmarks (ironic, isn't it?) generating the same charts took much longer with flot than with chart.js. That said, it still takes a non-trivial amount of time to render the benchmark link posted above on mobile. |
jonascarpay
force-pushed
the
master
branch
4 times, most recently
from
0bd58d3
to
271bbcf
Compare
I'm not sure I agree that this description is correct, the groups are sorted in ascending alphabetical order but we split the report name on '/' and compare the two right-most group elements first with (the default) ascending alphabetical string comparison, then if equal the group elements to the left of those are compared until one of the elements are unequal. If all elements are equal the shorter array is sorted before the longer one. This image provides a visualization of the lexical and colexical orderings. The right-most 1s are placed before the right-most 5s in colexical order. |
OK. My attempts at succinctly characterizing this have failed, so perhaps it's best that I stop trying. :) In that case, I'm content with the current description, but include a link to the Wikipedia article on lexicographic ordering you mentioned above, since that would be valuable context to have. |
|
One last request: do you mind writing up a description of the changes from this patch in the |
Yes, of course, I'll co-ordinate with @jonascarpay and describe the changes in the changelog either today or tomorrow. |
|
Alright, that's the Changelog. Do you want me to squash the history? |
Please do. And thanks again for all of your combined work on this! |
3249df7 add chart.js thing a806dfd Output table 424d0f7 WIP 4b72393 Add colophon and styling ec94eba Properly format scatter tooltip 60fdba8 Add outliers d51ffeb Add grokularation 9a8b08e Minimum bar width 0ca856b Remove stray console logs 73d573b Implement animating error bars 80da94a Replace annotation plugin with custom function 9be269c Fix groups and regression table 1c1b66a Make overview ticks robust 56a9375 Render 3 significant digits f4afc21 Update overview tick rendering 2886337 Add endpoint tick on logarithmic scale if within one order efd3b6b Fix hover on y-axis c543d8d Split chart.html into separate JS and CSS files 3f6d9a7 Update to use js-chart c14274c Remove backup file 0674754 Remove unused jQuery plugin 930da09 Update www/ with new reports d29acdd Flatten templates/js/ 21bebd8 Add groups field to report fda1377 Make drawErrorBar scope b65e7db Comment on criterion.js functions 9e719c8 Re-render report.html 2ad7928 cleanup 414911b Expand hover hit radius for points 543778c Add explanatory note about interactivity d6d5ce9 Clip error bars to chart area 0547c42 Use chart.js helper to get chart position ed2a680 Add legend that hides groups 7142270 Constrain legend box width 2eec914 Add titles and axes labels 64d9076 Don't display labels 910573b Fix "a" in outlier information 5c26f2c Fix wording ce402c4 Cater explanation depending on screen or print 8e501fe Make legend toggle-able 8da2146 Controls explanation and legend chevron cd2dd68 Avoid page breaks inside single report 7fc20ac Update grokularation with no-prints and scatter plot CI explanation fe49774 Colophon link color in printed version fc8690e Re-generate report.html and fibber.html 2d00dcd Render overview labels over bars on small screens 1ba261e Base error bars on bar index 271bbcf Remove `report` from JS context 446e2a6 Link to Wikipedia for (co)lexical order 198f1ff Update changelog for new HTML reports
|
Done, thank you as well! |
|
I’m so excited about this. Thank you all so much!
…On Sat, Nov 14, 2020 at 12:03 PM Jonas Carpay ***@***.***> wrote:
Done, thank you as well!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#229 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAABBQVEFUDV7CTZKXR72U3SP3BA5ANCNFSM4TNDRVBQ>
.
|
|
I've uploaded |
Originally intended to close #213, has turned into a rewrite using
chart.js.fibber.htmlreport.htmlFeedback, criticism, feature requests welcome!
To do
Report/template
(optional?) legends(collapsible) table of contents?Haskell+packaging
js-flot,js-jquerychart.jsjs-chart?@considerate @cartazio