-
-
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 improved ess rhat #3312
Conversation
…tan-dev/stan into feature/3299-improved-ESS-Rhat
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: |
this is ready for review. |
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: |
for (std::size_t k = i; k < j; ++k) { | ||
double p = (avg_rank - 0.375) / (size + 0.25); | ||
const Eigen::Index index = value_with_index[k].second; | ||
rank_matrix(index) = boost::math::quantile(dist, p); |
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.
am I correct that this is stan::math::inv_Phi
?
I know this is just moving code around, but if so that's probably worth calling instead
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.
I'm not sure - this is a question for @aleksgorica or @avehtari
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.
@WardBrian, can you clarify the question? I'm not certain to which part you refer to be stan::math::inv_Phi()
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.
We're computing rank[index] by using the boost::math::quantile function where this dist
object is just a standard normal -- I believe that is equivalent to calling our own inv_Phi
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 -- if we get an answer on that inv_Phi question we can address separately
Yes, Our internal https://mc-stan.org/math/prim_2fun_2inv___phi_8hpp_source.html and then in what looks like an identical implementation to me to first order, for OpenCL: https://mc-stan.org/math/opencl_2kernels_2device__functions_2inv___phi_8hpp_source.html |
* @param chains stores chains in columns | ||
* @return normal scores for average ranks of draws | ||
*/ | ||
inline Eigen::MatrixXd rank_transform(const Eigen::MatrixXd& chains) { |
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.
this function moved to file rank_normalization.hpp
@avehtari - in this PR, file stan/src/stan/analyze/mcmc/compute_effective_sample_size.hpp Lines 73 to 137 in 2550bbd
do you see any problems here? |
and I'm not able to figure in my head if that is guaranteed to give the same answer. I can't remember, but I assume I had a reason to have it outside that earlier loop and in the place where I had it.
and then later
|
re point 2 - yes, caught that. the differences between this implementation and posterior are that missing step:
and the use of these have been fixed in the latest version of PR #3313, and unit tests added which verify that the old and new versions of ess compute the same answer. I'm now going through the rank-normalized rhat and will add similar checks. this raises the question of the why a different implementation of covariance? is one ESS specific? |
Andrew instilled that distinction in me, and I try to stick to it, but I've given up trying to get other people to recognize the distinction. It's just too commonly used everywhere to fight. Maybe I should write another crabby linguist blog post :-) On the other hand, I think we should stick to this distinction in our own writing about Stan, at least in our doc. I created an issue to deal with the dozens of uses in our docs that violate the @andrewgelman style guide. |
Submission Checklist
./runTests.py src/test/unit
make cpplint
Summary
Refactor of method for computing rank-normalized split bulk and tail rhat and addition of method to compute rank-normalized split bulk and tail ess as described in https://arxiv.org/abs/1903.08008.
Intended Effect
Refactor the logic added to
stan/analyze/mcmc/compute_potential_scale_reduction.hpp
via PR #3266. so that some of the logic can be reused to implement rank-normalized split ESS.rank_normalization
andsplit_chains
and new functionsrank_normalized_split_rhat
andrank_normalized_split_ess
, each in a separate file.This PR will be followed by another refactor of the
stan::mcmc::chains
object. In anticipation of that refactoring, bothrank_normalized_split_rhat
andrank_normalized_split_ess
have a single argument which is an Eigen::MatrixXd of draws instead of vectorsdraws
,sizes
.How to Verify
Unit tests based on running the same Stan CSV files through CmdStanR and using the current implementations in R's posterior library to get rhat and ess.
Side Effects
N/A
Documentation
N/A - when CmdStan's
stansummary
method is updated, will be documented in CmdStan docs.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: