-
-
Notifications
You must be signed in to change notification settings - Fork 984
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
Numerically stabilize autocorrelation() #3114
Conversation
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.
is this the right fix? under what circumstances does autocorr[..., :1]
get small?
Yeah I don't know if this is the right fix. The issue is when one value doesn't vary at all, so in that case I think the autocorrelation should be 1. |
would it make sense to e.g. add an explicit test that checks that the resulting effective sample size is small for a variable with zero variance? |
OK I've added some tests.
hmm, it looks like our |
Thanks for reviewing! |
* Allow autocorrelation() to run without mkl * Numerically stabilize * Clarify definition, add test * Remove xfail_if_not_implemented
This sets the autocorrelation of constant series to be 1.0 rather than NAN.
I believe this is the proper behavior for MCMC testing, where series with no variation should be seen as having effective sample size of 1.
My use case is in computing autocorrelation of discrete time series, where I'm using autocorrelation as a diagnostic to detect lack of mixing. In that case I believe this PR is correct.
Tested