-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
CmdStanMCMC/MLE/VB from read_cmdstan_csv ouput #412
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rok-cesnovar Just added a bunch of review comments that hopefully help explain the changes.
DESCRIPTION
Outdated
@@ -1,6 +1,6 @@ | |||
Package: cmdstanr | |||
Title: R Interface to 'CmdStan' | |||
Version: 0.3.0 | |||
Version: 0.3.0.9000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bumping to dev version
@yizhang-yiz I think this is now ready to play around with if you want to try it out. |
> fit <- cmdstanr::as_cmdstan_mcmc(dir(pattern=".csv", full.name=TRUE)) This works bayesplot::mcmc_dens_overlay(fit$draws(), pars = c("CL", "Q", "V1", "V2", "ka", "sigma"), facet_args = list(nrow = 2)) but this doesn't > print(fit$time(), digits = 3)
Error: This method is not available for objects created using as_cmdstan_mcmc(). I wonder what the rationale is. |
No good one other than it would take more effort to get that to work. Lines 804 to 806 in 46ae80f
which handles preparing the call to cmdstan, setting up the external processes, and then running cmdstan. It's also the object that report the run times (because we don't wait for reading in the csv to report the time). The way I created |
Agree. Will do that separately. |
Codecov Report
@@ Coverage Diff @@
## master #412 +/- ##
==========================================
+ Coverage 88.29% 88.45% +0.15%
==========================================
Files 12 12
Lines 2793 2823 +30
==========================================
+ Hits 2466 2497 +31
+ Misses 327 326 -1
Continue to review full report at Codecov.
|
I changed to Let's wait until #414 is merged and then we can update this PR to include |
Ok I changed the class names to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #412 +/- ##
==========================================
+ Coverage 88.29% 88.45% +0.15%
==========================================
Files 12 12
Lines 2793 2823 +30
==========================================
+ Hits 2466 2497 +31
+ Misses 327 326 -1 ☔ View full report in Codecov by Sentry. |
Submission Checklist
Summary
Closes #411
Allows creating CmdStanMCMC, CmdStanVB, and CmdStanMLE objects from the list returned by
read_cmdstan_csv()
.In reality these objects are not exactly the same because they don't have an associated CmdStanRun object. This means that methods like
cmdstan_diagnose()
andsave_output_files()
are unavailable, but the most important methods (e.g.,draws()
,summary()
) work fine.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):
Columbia University
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses: