Closed mitzimorris closed 1 month ago
Name | Old Result | New Result | Ratio | Performance change( 1 - new / old ) |
---|---|---|---|---|
arma/arma.stan | 0.36 | 0.36 | 0.99 | -1.3% slower |
low_dim_corr_gauss/low_dim_corr_gauss.stan | 0.01 | 0.01 | 1.0 | 0.11% faster |
gp_regr/gen_gp_data.stan | 0.03 | 0.03 | 0.99 | -0.53% slower |
gp_regr/gp_regr.stan | 0.09 | 0.1 | 0.99 | -1.1% slower |
sir/sir.stan | 73.32 | 72.86 | 1.01 | 0.63% faster |
irt_2pl/irt_2pl.stan | 4.41 | 4.26 | 1.04 | 3.57% faster |
eight_schools/eight_schools.stan | 0.06 | 0.06 | 1.07 | 6.75% faster |
pkpd/sim_one_comp_mm_elim_abs.stan | 0.26 | 0.25 | 1.05 | 4.92% faster |
pkpd/one_comp_mm_elim_abs.stan | 20.2 | 19.02 | 1.06 | 5.86% faster |
garch/garch.stan | 0.46 | 0.42 | 1.08 | 7.49% faster |
low_dim_gauss_mix/low_dim_gauss_mix.stan | 2.82 | 2.65 | 1.06 | 6.09% faster |
arK/arK.stan | 1.89 | 1.74 | 1.08 | 7.72% faster |
gp_pois_regr/gp_pois_regr.stan | 2.9 | 2.73 | 1.06 | 5.97% faster |
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan | 9.16 | 8.55 | 1.07 | 6.62% faster |
performance.compilation | 199.22 | 190.83 | 1.04 | 4.21% faster |
Mean result: 1.0406367197239463
Jenkins Console Log Blue Ocean Commit hash: d669f2b029575d5191b8f394b58b652e4bc4c805
this is ready for review.
Name | Old Result | New Result | Ratio | Performance change( 1 - new / old ) |
---|---|---|---|---|
arma/arma.stan | 0.46 | 0.39 | 1.19 | 15.93% faster |
low_dim_corr_gauss/low_dim_corr_gauss.stan | 0.01 | 0.01 | 1.04 | 4.06% faster |
gp_regr/gen_gp_data.stan | 0.03 | 0.03 | 1.06 | 5.56% faster |
gp_regr/gp_regr.stan | 0.1 | 0.1 | 1.03 | 2.98% faster |
sir/sir.stan | 78.92 | 74.95 | 1.05 | 5.02% faster |
irt_2pl/irt_2pl.stan | 5.92 | 4.9 | 1.21 | 17.12% faster |
eight_schools/eight_schools.stan | 0.06 | 0.06 | 1.02 | 2.24% faster |
pkpd/sim_one_comp_mm_elim_abs.stan | 0.28 | 0.26 | 1.07 | 6.21% faster |
pkpd/one_comp_mm_elim_abs.stan | 21.15 | 20.52 | 1.03 | 2.98% faster |
garch/garch.stan | 0.52 | 0.46 | 1.11 | 10.17% faster |
low_dim_gauss_mix/low_dim_gauss_mix.stan | 3.31 | 2.82 | 1.17 | 14.78% faster |
arK/arK.stan | 2.0 | 1.87 | 1.07 | 6.64% faster |
gp_pois_regr/gp_pois_regr.stan | 3.18 | 2.94 | 1.08 | 7.59% faster |
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan | 9.77 | 9.19 | 1.06 | 5.93% faster |
performance.compilation | 203.68 | 197.84 | 1.03 | 2.87% faster |
Mean result: 1.0822159046829158
Jenkins Console Log Blue Ocean Commit hash: d669f2b029575d5191b8f394b58b652e4bc4c805
Looks good -- if we get an answer on that inv_Phi question we can address separately
Yes, inv_Phi
is the quantile function for standard normal distributions, so this should be the same.
Our internal inv_Phi
is hard coded directly:
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
@avehtari - in this PR, file split_rank_normalized_ess.hpp
, function ess
is a refactor of this: https://github.com/stan-dev/stan/blob/2550bbd2bbde86352b9cf9e1bc520968e3a88788/src/stan/analyze/mcmc/compute_effective_sample_size.hpp#L73-L137
do you see any problems here?
Why ess
function is in split_rank_normalize_ess.hpp
while it's needed also for non-rank-normalized ess?
The new code is dropping the following and I don't understand how it can be done
for (int chain = 0; chain < num_chains; ++chain)
acov_s(chain) = acov(chain)(1);
The initial monotone sequence part has been moved before "the improved estimate" lines
// this is used in the improved estimate, which reduces variance
// in antithetic case -- see tau_hat below
if (rho_hat_even > 0)
rho_hat_s(max_s + 1) = rho_hat_even;
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.
I (and Andrew) don't like num_samples
. It should be either num_draws
, num_total_draws
, or sample_size
. The best would be to have in the beginning
const Eigen::Index num_chains = chains.cols();
const Eigen::Index num_iterations = chains.rows();
and then later
double num_draws = num_chains * num_iterations;
re point 2 - yes, caught that.
the differences between this implementation and posterior are that missing step:
for (int chain = 0; chain < num_chains; ++chain)
acov_s(chain) = acov(chain)(1);
and the use of stan::analyze::covariance
instead of stan::math::covariance
.
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?
I (and Andrew) don't like num_samples
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 https://github.com/stan-dev/stan/pull/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: