-
-
Notifications
You must be signed in to change notification settings - Fork 371
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
Feature/3299 improve ess mcse #3305
Conversation
…hub.com/stan-dev/stan into feature/3299-improve-ESS-MCSE
…hub.com/stan-dev/stan into feature/3299-improve-ESS-MCSE
…re/3299-improve-ESS-MCSE
@SteveBronder - I would really appreciate your eyeballs on the ESS calculations - a preliminary review? @WardBrian - does this API look OK? also @avehtari - I implemented tail ESS - current logic is that if tail ESS returns NaN, set if to bulk ESS, as this seems to be what the posterior package does. is this correct? the current set of unit tests test split R-hat and split-ESS against what's in posterior for a run of 2 chains on the eight_schools model. suggestions for further tests welcome. |
…an-dev/stan into feature/3299-improve-ESS-MCSE
…an-dev/stan into feature/3299-improve-ESS-MCSE
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
…an-dev/stan into feature/3299-improve-ESS-MCSE
…an-dev/stan into feature/3299-improve-ESS-MCSE
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
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.
Thanks for tackling this
src/test/unit/analyze/mcmc/compute_potential_scale_reduction_test.cpp
Outdated
Show resolved
Hide resolved
…an-dev/stan into feature/3299-improve-ESS-MCSE
…re/3299-improve-ESS-MCSE
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.
Few comments about chainset
explicit chainset(const std::vector<stan::io::stan_csv>& stan_csv) { | ||
if (stan_csv.empty()) | ||
return; | ||
init_from_stan_csv(stan_csv[0]); | ||
for (size_t i = 1; i < stan_csv.size(); ++i) { | ||
add(stan_csv[i]); | ||
} | ||
} |
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.
Can we write a thinned_samples
function with a signature thinned_samples(const std::vector<stan::io::stan_csv>&)
? Also add a function like the below so that we can use a member initialization list
std::vector<Eigen::MatrixXd> extract_samples(const std::vector<stan::io::stan_csv>&)`
explicit chainset(const std::vector<stan::io::stan_csv>& stan_csv) { | |
if (stan_csv.empty()) | |
return; | |
init_from_stan_csv(stan_csv[0]); | |
for (size_t i = 1; i < stan_csv.size(); ++i) { | |
add(stan_csv[i]); | |
} | |
} | |
explicit chainset(const std::vector<stan::io::stan_csv>& stan_csvs) : | |
param_names_(stan_csvs[0].header), | |
num_samples_(thinned_samples(stan_csvs)), | |
chains_(extract_samples(stan_csvs)) {} |
inline int num_chains() const { return chains_.size(); } | ||
|
||
/** | ||
* Report number of parameters per chain. | ||
* @return size of parameter names vector. | ||
*/ | ||
inline int num_params() const { return param_names_.size(); } | ||
|
||
/** | ||
* Report number of samples (draws) per chain. | ||
* @return rows per chain | ||
*/ | ||
inline int num_samples() const { return num_samples_; } |
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.
std::vector
returns size_t
which is a unsigned int
. I like using Eigen::Index
instead of int
since it's a long int
and it's a reasonably safe int to convert to
inline int num_chains() const { return chains_.size(); } | |
/** | |
* Report number of parameters per chain. | |
* @return size of parameter names vector. | |
*/ | |
inline int num_params() const { return param_names_.size(); } | |
/** | |
* Report number of samples (draws) per chain. | |
* @return rows per chain | |
*/ | |
inline int num_samples() const { return num_samples_; } | |
inline int num_chains() const { return chains_.size(); } | |
/** | |
* Report number of parameters per chain. | |
* @return size of parameter names vector. | |
*/ | |
inline int num_params() const { return param_names_.size(); } | |
/** | |
* Report number of samples (draws) per chain. | |
* @return rows per chain | |
*/ | |
inline int num_samples() const { return num_samples_; } |
…an-dev/stan into feature/3299-improve-ESS-MCSE
…an-dev/stan into feature/3299-improve-ESS-MCSE
closing this PR - see comment #3310 (comment) |
Submission Checklist
./runTests.py src/test/unit
make cpplint
Summary
Add split-rank-folded ESS .
Intended Effect
Expose new split-Rhat and split-ESS for CmdStan
How to Verify
unit tests
Side Effects
N/A
Documentation
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: