Benchmark #46
Benchmark #46
Conversation
|
This PR adds some arithmetic benchmarks from AGK18. We plan to make a second PR to reproduce Tables 3-6 in the same paper that uses some of the tests defined in Project Daisy. |
|
@aadesha Thanks for your contribution. Is this ready for review? |
|
It's ready, we are going to add another set of benchmarks too. Probably
tomorrow.
…On Fri, 7 Jun, 2019, 10:05 PM Luis Benet, ***@***.***> wrote:
@aadesha <https://github.com/Aadesha> Thanks for your contribution. Is
this ready for review?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#46?email_source=notifications&email_token=AHBGTUQWSKP5RSBW2E6LZHLPZKE45A5CNFSM4HVXHWIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXGK7TY#issuecomment-499953615>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHBGTUSH54BRFSGT6QQOVS3PZKE45ANCNFSM4HVXHWIA>
.
|
In a separate PR. This one is ready for review. |
There was a problem hiding this comment.
In general, it looks good to me. I have few general comments though:
-
I think it is to have a
Benchmark/README.mdfile, with some general info about the benchmarks (e.g., how to run them), and perhaps also give a summary of the results or pointers to some pages where the actual results are. This should be easy -
Rename
Benchmarksdirectory toBenchmarks(note the final "s"), and perhaps use lower case. Maybe @dpsanders has something to say as a native speaker... -
Add separately the scalar cases as well.
-
It is also a good idea to have an automatic way to create and display the (bechmark) results. I am thinking of using Weave.jl or Literate.jl. (Perhaps it is too much to ask for this PR, but well-worth the effort for maintenance.
|
For the last point of my comment, see SciML/SciMLBenchmarks.jl#32 |
+1 This file has the basic commands to run and display results; @aadesha could you collect them in
The filename and folder are the ones adopted by convention in
+1 IIUC, you'd like to have (in addition to the ones added here)
That would be awesome; export_markdown("results.md", results) works but one has to manually plug that output into a document. |
Yes, better leave that for later.. |
|
I agree with what all of you are saying,
I will take care of
1) README.
2) Lowercase 's'.
3) adding operations on scalar.
4)and Try for last one.
…On Fri, 7 Jun, 2019, 10:38 PM Marcelo Forets, ***@***.***> wrote:
I think it is to have a Benchmark/README.md file,
+1 This file
<https://github.com/JuliaReach/LazySets.jl/blob/master/benchmark/benchmarks.jl#L2>
has the basic commands to run and display results; @aadesha
<https://github.com/Aadesha> could you collect them in benchmark/README.md
?
Rename Benchmarks directory to Benchmarks (note the final "s"), and
perhaps use lower case. Maybe @dpsanders <https://github.com/dpsanders>
has something to say as a native speaker...
The filename and folder are the ones adopted by convention in
PkgBenchmarks, however it should be benchmark ***@***.***
<https://github.com/Aadesha> mind the lower case), see this doc
<https://juliaci.github.io/PkgBenchmark.jl/stable/define_benchmarks.html#Defining-a-benchmark-suite-1>
.
Add separately the scalar cases as well.
+1 IIUC, you'd like to have (in addition to the ones added here) @benchmarkable
$b1 + $b2 and so on.
It is also a good idea to have an automatic way to create and display the
(bechmark) results. I am thinking of using Weave.jl or Literate.jl.
(Perhaps it is too much to ask for this PR, but well-worth the effort for
maintenance.
That would be awesome; export_markdown("results.md", results)
<https://juliaci.github.io/PkgBenchmark.jl/stable/export_markdown.html>
works but one has to manually plug that output into a document.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#46?email_source=notifications&email_token=AHBGTUUVMLI3MV52WPD3WWTPZKI2FA5CNFSM4HVXHWIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXGNVCQ#issuecomment-499964554>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHBGTUX5N25MDBDXSD7MJGDPZKI2FANCNFSM4HVXHWIA>
.
|
|
The plan LGTM: address the first three points, and open an issue with the fourth one, so we don't forget about it. |
|
That will be more appropriate, yes.
…On Fri, 7 Jun, 2019, 10:49 PM Luis Benet, ***@***.***> wrote:
The plan LGTM: address the first three points, and open an issue with the
fourth one, so we don't forget about it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#46?email_source=notifications&email_token=AHBGTUXUK3VLYMRV3AHTXQ3PZKKCPA5CNFSM4HVXHWIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXGOQUI#issuecomment-499968081>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHBGTUXO6FI4BRHPF2EFWX3PZKKCPANCNFSM4HVXHWIA>
.
|
|
PR is ready for review. |
There was a problem hiding this comment.
I have pointed out to a typo in the README.md guide.
Aside form this, I think the directory name should be benchmark instead of Benchmark; in my Mac this causes no problem, but it may if you on a linux box (I haven't checked it). I have no way to test this, but I could do it later.
|
Thanks a lot! I'm merging |
No description provided.