Skip to content
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

Change n_eff to ESS #192

Open
avehtari opened this issue Dec 2, 2021 · 5 comments
Open

Change n_eff to ESS #192

avehtari opened this issue Dec 2, 2021 · 5 comments

Comments

@avehtari
Copy link
Collaborator

avehtari commented Dec 2, 2021

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
Copy link
Collaborator Author

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

@avehtari
Copy link
Collaborator Author

@jgabry any comment on this?

@avehtari avehtari changed the title Chnage n_eff to ESS Change n_eff to ESS Mar 24, 2023
@jgabry
Copy link
Member

jgabry commented Mar 24, 2023

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
Copy link
Member

jgabry commented Mar 24, 2023

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
Copy link
Collaborator Author

avehtari commented Mar 5, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants