Skip to content

use CSV/stdout reported time for time()#307

Merged
jgabry merged 6 commits intomasterfrom
use_reported_time
Oct 7, 2020
Merged

use CSV/stdout reported time for time()#307
jgabry merged 6 commits intomasterfrom
use_reported_time

Conversation

@rok-cesnovar
Copy link
Copy Markdown
Member

@rok-cesnovar rok-cesnovar commented Oct 5, 2020

Submission Checklist

  • Run unit tests
  • Declare copyright holder and agree to license (see below)

Summary

Fixes #303

> fit$time()
$total
[1] 0.9245193

$chains
  chain_id warmup sampling total
1        1  0.026    0.375 0.401
2        2  0.017    0.417 0.434
3        3  0.017    0.383 0.400
4        4  0.018    0.398 0.416

which matches the CSV reported times.

For fixed_param

> fit$time()
$total
[1] 0.1305542

$chains
  chain_id warmup sampling total
1        1      0    0.111 0.111

warmup is 0, which is correct as @yizhang-yiz points out.

$time()$total is still the Sys.time()-timed execution time. That is the time to execute all chains and will differ depending on how many chains you run in parallel. Non-sampling methods do not have time reports in the CSV or stdout so those still use Sys.time() as well.

$time()$chains reports times reported in stdout (CSV).

Copyright and Licensing

Please list the copyright holder for the work you are submitting
(this will be you or your assignee, such as a university or company):
Rok Češnovar, Univ. of Ljubljana

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses:

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 6, 2020

Codecov Report

Merging #307 into master will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #307      +/-   ##
==========================================
- Coverage   90.15%   90.12%   -0.03%     
==========================================
  Files          12       12              
  Lines        2428     2421       -7     
==========================================
- Hits         2189     2182       -7     
  Misses        239      239              
Impacted Files Coverage Δ
R/run.R 97.08% <100.00%> (-0.05%) ⬇️

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 878da87...1d0bff9. Read the comment docs.

@rok-cesnovar
Copy link
Copy Markdown
Member Author

Will add a simple test just for sanity check that CSV times match but otherwise this is ready for review.

@rok-cesnovar rok-cesnovar requested a review from jgabry October 6, 2020 07:38
@jgabry jgabry merged commit 495be54 into master Oct 7, 2020
@jgabry jgabry deleted the use_reported_time branch October 7, 2020 21:40
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.

incorrect timing report

3 participants