search

stan-dev / loo

loo R package for approximate leave-one-out cross-validation (LOO-CV) and Pareto smoothed importance sampling (PSIS)
https://mc-stan.org/loo
Other
149 stars 34 forks source link

Change n_eff to ESS #192

Open avehtari opened 3 years ago

avehtari commented 3 years ago

The current output

Pareto k diagnostic values:
                         Count Pct.    Min. n_eff
(-Inf, 0.5]   (good)     717   73.2%   151       
 (0.5, 0.7]   (ok)       133   13.6%   116       
   (0.7, 1]   (bad)       74    7.6%   38        
   (1, Inf)   (very bad)  56    5.7%   839       
See help('pareto-k-diagnostic') for details.

has column n_eff. Following new Rhat paper and posterior package, I propose we change that to ESS as in effective sample size.

avehtari commented 2 years ago

@jgabry I would like to change also loo object part diagnostics$n_eff to diagnostics$ess, but can we do that?

avehtari commented 1 year ago

@jgabry any comment on this?

jgabry commented 1 year ago

I agree that it would be nice to change to ess, but it's a bit tricky. The problem is that the diagnostics element of the loo and psis objects doesn't have its own class (it's just a list), which means we can't define custom methods for [, [[, and $ that throw a deprecation warning when diagnostics$n_eff is accessed.

If we wanted to make this change I think the first step would be to add a class to the diagnostics list in one release and then in a later release we could add the new ess slot and methods for [, [[, and $ that throw a warning if n_eff is accessed (but still return n_eff to avoid breaking people's code).

I'm not sure that it's worth it, but maybe it is. What do you think?

Another question is what to do with the r_eff argument that we have in many functions. The name is consistent with n_eff. Would we keep that or change it to something else too?

jgabry commented 1 year ago

If we wanted to make this change I think the first step would be to add a class to the diagnostics list in one release and then in a later release we could add the new ess slot and methods for [, [[, and $ that throw a warning if n_eff is accessed (but still return n_eff to avoid breaking people's code).

I meant to say we could add the class plus the ess slot in one release (so both ess and n_eff work without warning for a little while), and then deprecate n_eff in a later release with a warning. But I guess we could do it all in the same release instead of two steps.

avehtari commented 9 months ago

The slot is still n_eff but #235 did change the print method to print ESS