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

reorganize time/ to benchmark/ and set up CI #50

Merged
merged 2 commits into from
Jul 14, 2020

Conversation

johnnychen94
Copy link
Contributor

  • test related codes are removed; code logic should be tested in test stage instead of benchmark stage
  • remove BenchmarkTools dependency from Project.toml; this package should not be a dependency of MIRT
  • use @benchmarkable instead of @btime as the former can be used by BenchmarkPkg and BenchmarkCI
  • Set up a Benchmark CI that get triggered when you label the PR with run benchmark

The benchmark CI could error in this PR if gets triggered because currently benchmark/ doesn't exist in master branch

* test related codes are removed; code logic should be tested in test
  stage instead of benchmark stage
* remove `BenchmarkTools` dependency from Project.toml; this package
  should not be a dependency of `MIRT`
* use `@benchmarkable` instead of `@btime` as the former can be used
  by BenchmarkPkg and BenchmarkCI
* Set up a Benchmark CI that get triggered when you label the PR with
  `run benchmark`
@codecov
Copy link

codecov bot commented Jul 12, 2020

Codecov Report

Merging #50 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
- Coverage   98.86%   98.81%   -0.05%     
==========================================
  Files          48       48              
  Lines        2107     2107              
==========================================
- Hits         2083     2082       -1     
- Misses         24       25       +1     
Impacted Files Coverage Δ
src/mri/mri_objects.jl 99.38% <0.00%> (-0.62%) ⬇️
src/plot/jim.jl 100.00% <0.00%> (ø)
src/fbp/sino_geom.jl 100.00% <0.00%> (ø)
src/fbp/image_geom.jl 100.00% <0.00%> (ø)
src/mri/mri_trajectory.jl 100.00% <0.00%> (ø)
src/utility/downsample.jl 100.00% <0.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 56ca418...8bc50d0. Read the comment docs.

@JeffFessler
Copy link
Owner

@johnnychen94, the julia code in the time/ directory is some simple timing tests to compare different versions of certain operations like downsampling. After I run those tests during development and determine which version is faster, then I just export the faster version in the package. I like to leave the timing code in the package as a reminder to myself for later how I chose between different options, and I also thought maybe someone else would want to test to see if the version I chose was faster on their platform and if not they could report an issue if a different version is faster.
At first I thought this PR would cause a benchmark run in the cloud every time we do a PR, which would waste energy. Now I understand that it will be invoked only with run benchmark as a label, so that is reasonable.
However, I do want to keep the files I had in the time/ directory so I will restore those after merging.

@JeffFessler JeffFessler merged commit 1221a51 into JeffFessler:master Jul 14, 2020
@johnnychen94 johnnychen94 deleted the jc/benchmarkci branch July 15, 2020 00:12
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.

None yet

2 participants