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

benchmarks of major functionality and fix LTO check/testthat errors #151

Merged
merged 20 commits into from
Oct 20, 2021

Conversation

slwu89
Copy link
Collaborator

@slwu89 slwu89 commented Oct 18, 2021

hey @giovannic!

I added benchmarking scripts to tests/performance using the bench package. I don't know what's the best way to set up these scripts so please let me know what you think after you have a chance to run them. Anyway, we probably won't be running benchmarks for every PR so it might not be a big deal.

I think I fixed the testthat error contributing to the LTO check problem on CRAN. Upgrading testthat to the latest version and setting up the C++ testing files again using testthat::use_catch() seems to have made changes suggested by OP in r-lib/testthat#1230. So maybe they fixed it and didn't remember to update people following the issue. After making that change I ran the r-hub builders, making sure --enable-lto flags were enabled and none of the build platforms had that specific error we were getting.

I also added #include <unordered_set> to Event.h as some of the r-hub platforms (Solaris .........) errored out without that header, and also deleted the ; from the ctor of TargetedEvent to get rid of that -Wpedantic compiler warning.

@slwu89 slwu89 requested a review from giovannic October 18, 2021 22:26
@codecov
Copy link

codecov bot commented Oct 18, 2021

Codecov Report

Merging #151 (1ad60f3) into dev (70967d8) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev     #151   +/-   ##
=======================================
  Coverage   94.68%   94.68%           
=======================================
  Files          26       26           
  Lines        1185     1185           
=======================================
  Hits         1122     1122           
  Misses         63       63           
Impacted Files Coverage Δ
R/categorical_variable.R 100.00% <100.00%> (ø)
inst/include/Event.h 92.68% <100.00%> (ø)
src/categorical_variable.cpp 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5667e0...1ad60f3. Read the comment docs.

Copy link
Member

@giovannic giovannic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Some suggestions:

It would be good to add bench to DESCRIPTION > Suggests.

Do we really need tidyr? It takes a while for me to install

It would be tidier to have build_grid, simplify_bench, create_random... in a tests/performance/utils.R file.

Thank you so much for the testthat fix!

@slwu89
Copy link
Collaborator Author

slwu89 commented Oct 19, 2021

Thanks @giovannic.

Yeah, let me see if I can work out a version of simplify_bench_output not relying on tidyr. I agree it would be much tidyr to put all that in a utils.R file as well.

@slwu89 slwu89 requested a review from giovannic October 19, 2021 18:28
Copy link
Member

@giovannic giovannic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Thanks!

@slwu89 slwu89 merged commit dd69dde into mrc-ide:dev Oct 20, 2021
@slwu89 slwu89 deleted the feat/bench-test branch October 20, 2021 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants