benchmark: allow to add lines to scatter plots#22074
benchmark: allow to add lines to scatter plots#22074lundibundi wants to merge 2 commits intonodejs:masterfrom
Conversation
AndreasMadsen
left a comment
There was a problem hiding this comment.
LGTM, although I don't see why it needs to be an option.
|
Making it non-optional is okay with me too, as it's hard to understand the plot based on dots-colors alone. I just didn't want to disrupt those who already use it too much. I'll add/change the doc example once this is decided then. |
|
@lundibundi I wouldn't bother. Very few people know about this tool, even less uses it. In fact, before you, I'm was the only contributor to it. |
TimothyGu
left a comment
There was a problem hiding this comment.
+1 for just make it always on.
5245721 to
37b717c
Compare
|
Now that I think of it, how should I update the plot in benchmarks guide, will it be okay to just put node master string_decode benchmarks there? (Also, maybe csv used in there is available somewhere?). |
Yeah, if you could just rerun the benchmark that generated that plot and update the graph that'd be great. I don't think the CSV used for the existing graph is available anywhere as the data can be regenerated quickly. |
37b717c to
d1da6de
Compare
|
Ok, I've updated the doc. PTAL. |
|
@lundibundi Nah, these files don't get touched often. Plus we can always regenerate. |
|
CI failed entirely, running again: https://ci.nodejs.org/job/node-test-pull-request/16385/ |
|
@lundibundi ... can you rebase this off of current master. There are some changes that need to be pulled in before this will pass CI |
d1da6de to
f352a42
Compare
|
@jasnell oh, forgot about that, thanks. |
Adds lines between the points of the same category in scatter.R plots.
f352a42 to
b15fb2d
Compare
|
One more: https://ci.nodejs.org/job/node-test-pull-request/16772/ Known flaky async-hooks/test-callback-error (#15985) failed. |
|
Landed in 7aac706 |
Adds lines between the points of the same category in scatter.R plots. PR-URL: #22074 Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Adds lines between the points of the same category in scatter.R plots. PR-URL: #22074 Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Adds lines between the points of the same category in scatter.R plots. PR-URL: #22074 Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Adds lines between the points of the same category in scatter.R plots. PR-URL: #22074 Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Adds --add-lines option to scatter.R that will add lines between the points of the same category.
It was helpful for me to see a general trend with different options, so I added this.
Also, this is my first time working with R so please be thorough, maybe I've missed something.
Checklist
make -j4 test(UNIX) passes/cc @nodejs/benchmarking