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

BDA3 Chapter 3 discussion of Effective sample size is flawed (needs update) #78

Open
rpgoldman opened this issue Apr 12, 2020 · 8 comments
Assignees

Comments

@rpgoldman
Copy link
Contributor

rpgoldman commented Apr 12, 2020

Problem

The discussion of number of effective samples/effective sample size in this notebook is unclear. I could possibly help clarify it, but I don't know enough to write it all myself.

How to replicate

I was running through the notebook for Chapter 3, and examining the "Example. Pre-election polling" section. This uses a no longer available PyMC3 API function to compute n_eff. I tried changing this to use the corresponding arviz function az.stats.ess, and got the anomalous results in the following screen-grab.

image

Suggested Improvements

  1. It would be very helpful to explain what is going on here. I assume that the number of effective samples aligns with the notion of ess_bulk. I think the discussion should be tweaked to make this point. I initially assumed that the return of as.stats.ess() would correspond to the mean ESS, rather than the "bulk."

  2. Given that the notion of ESS is nowhere discussed in Chapter Three of BDA, it would be a good idea to explain it here, although I suppose the reader is able to get the "take home" message that the ESS is low and this is diagnostic of bad sampling. But we don't even say that the ESS is a diagnostic measure.

  3. Another thing is that the textual discussion uses n_eff and "number of effective samples" instead of "effective sample size," which is confusing.

@aloctavodia
Copy link
Member

Hi @rpgoldman, notice that the default method for az.ess is bulk and those values agree (within the rounding error) with the ones reported by pm.summary in your example.

I agree we should update the examples and we should also directly use arviz functions when possible.

@rpgoldman
Copy link
Contributor Author

@aloctavodia Hi. I just updated my issue report as I discovered that the ESS was "bulk," not "mean." I also tried to more clearly state what I think is the problem and what I think could be done to improve matters.

A person who has only gotten to chapter 3 of BDA is likely not to understand how sampling should be diagnosed...

@rpgoldman
Copy link
Contributor Author

Also note that we must use the arviz function here -- the PyMC3 one no longer exists (or it has been renamed).

@rpgoldman
Copy link
Contributor Author

@aloctavodia BTW, this is leading me to patch the Arviz docs for the ess function. One thing that is odd in there is that it describes the var_names as

Names of variables to include in the effective_sample_size_mean report

I think this should just be "names of variables for which to compute ESS", right? I'm betting the effective_sample_size_mean is obsolete, since we offer multiple methods, not just mean.

@aloctavodia
Copy link
Member

@rpgoldman I totally agree with the statement "A person who has only gotten to chapter 3 of BDA is likely not to understand how sampling should be diagnosed..." But I think the main purpose of these "resources" is to provide a port of the code used in BDA and other books to PyMC3/4/ArviZ, not sure we should "complement" the content of those books here. Maybe we can add a few lines saying something about ess but a more detailed explanation belong somewhere else maybe we can add a link to this material https://github.com/arviz-devs/Exploratory-Analysis-of-Bayesian-Models/tree/master/content (that is far from ready) but it's main intention is precisely to explain the "theory" and "good practices" around methods implemented in ArviZ.

Right, the value returned by ESS is "bulk," "mean" is the value of the old way of computing ess. Probably we should revisit winch values we show in summary, as this can be certainly confusing and also address the issue with the docstring you mention. This all are important issues and we should addressed.

Are you willing to send a PR to fix this issue?

@rpgoldman
Copy link
Contributor Author

Sure, I'll try a PR, but it will definitely need a review!

@rpgoldman
Copy link
Contributor Author

@aloctavodia If you get a chance, please see my Arviz docstring fix (I don't have permissions to assign it to you for review).

@aloctavodia
Copy link
Member

Sure, thanks! I will do.

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

3 participants