Skip to content

Fix returning draws/sampling diagnostics for warmup with no samples#293

Merged
jgabry merged 6 commits intomasterfrom
bugfix/no_samples_diag
Aug 27, 2020
Merged

Fix returning draws/sampling diagnostics for warmup with no samples#293
jgabry merged 6 commits intomasterfrom
bugfix/no_samples_diag

Conversation

@rok-cesnovar
Copy link
Copy Markdown
Member

Submission Checklist

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

Summary

Fixes #288

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:

Comment thread R/fit.R
private$metadata_ <- data_csv$metadata

if (!is.null(data_csv$post_warmup_draws)) {
missing_variables <- !(posterior::variables(data_csv$post_warmup_draws) %in% posterior::variables(private$draws_))
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This fixes an issue that is related to the main issue this PR is fixing. It was a bug all along but only surfaced with this.

library(cmdstanr)
stan_program <- file.path(cmdstan_path(), "examples/bernoulli/bernoulli.stan")
model <- cmdstan_model(stan_program)

stan_data <- list(N = 10, y = c(0,1,0,0,0,0,0,0,0,1))
fit3 = model$sample(data = stan_data, parallel_chains = 4, save_warmup = 1, iter_warmup = 100, iter_sampling = 0, save_latent_dynamics = TRUE)

fit3$draws() #works fine
fit3$draws() #fails

fit3$sampler_diagnostics() #works fine
fit3$sampler_diagnostics() #fails

y1 <- fit_mcmc_1$sampler_diagnostics(inc_warmup = FALSE)
y2 <- fit_mcmc_1$sampler_diagnostics(inc_warmup = TRUE)
y3 <- fit_mcmc_3$sampler_diagnostics(inc_warmup = TRUE)
y4 <- fit_mcmc_3$sampler_diagnostics(inc_warmup = FALSE)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Calling sampler_diagnostics() on the same fit would without https://github.com/stan-dev/cmdstanr/pull/293/files#r478645723

@rok-cesnovar
Copy link
Copy Markdown
Member Author

Both this PR and #294 are ready for review.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 27, 2020

Codecov Report

Merging #293 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #293      +/-   ##
==========================================
+ Coverage   90.12%   90.15%   +0.02%     
==========================================
  Files          12       12              
  Lines        2421     2427       +6     
==========================================
+ Hits         2182     2188       +6     
  Misses        239      239              
Impacted Files Coverage Δ
R/fit.R 98.25% <100.00%> (+0.03%) ⬆️
R/read_csv.R 96.30% <100.00%> (+0.02%) ⬆️

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 0e3069d...602b805. Read the comment docs.

Copy link
Copy Markdown
Member

@jgabry jgabry left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Some checks are still running but assuming they pass we can merge.

@jgabry jgabry merged commit f58f017 into master Aug 27, 2020
@jgabry jgabry deleted the bugfix/no_samples_diag branch August 27, 2020 21:01
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.

fit$sampler_diagnostics() not working correctly when iter_sampling = 0

3 participants