-
-
Notifications
You must be signed in to change notification settings - Fork 379
Feature/3299 improve ess mcse #3305
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
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: |
WardBrian
left a comment
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
…an-dev/stan into feature/3299-improve-ESS-MCSE
…re/3299-improve-ESS-MCSE
SteveBronder
left a comment
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/unitmake cpplintSummary
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: