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

use biased autocorrelation estimate by default (resolves #1785) #1877

Merged
merged 3 commits into from
Oct 6, 2024

Conversation

jonny-so
Copy link
Contributor

@jonny-so jonny-so commented Oct 5, 2024

This changes the autocorrelation function to use the same (biased) estimate as Stan by default. The unbiased estimate is known to have "wild" tails, with variance that is O(1) at longer lags regardless of the length of the chain. See Priestley (1981, Section 5.3) for a discussion of the properties of these estimators. Note that the old (unbiased) estimator is only strictly unbiased when the mean is known, but I have followed Priestley (1981) in referring to it as unbiased.

I have added two checks to test_autocorrelation that highlight the difference in the tail behaviour.

Copy link
Member

@fehiepsi fehiepsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know how this change will affect ESS of the current users' experiments?

@@ -96,12 +96,13 @@ def _fft_next_fast_len(target):
target += 1


def autocorrelation(x, axis=0):
def autocorrelation(x, axis=0, unbiased=False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe change unbiased to bias and set bias=True by default.

"""
Computes the autocorrelation of samples at dimension ``axis``.

:param numpy.ndarray x: the input array.
:param int axis: the dimension to calculate autocorrelation.
:param unbiased: use an unbiased estimator of the autocorrelation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whether to use a biased estimator

@jonny-so
Copy link
Contributor Author

jonny-so commented Oct 5, 2024

Do you know how this change will affect ESS of the current users' experiments?

It will change it for sure, my experience with the current ESS was that it was too erratic to even be useful, which is why I started looking into this in the first place. If it is a concern we could always make bias=False the default, perhaps with a warning in the comments. My view is that given it is diagnostic code it is relatively low risk to change the default, but I'm happy to go with either.

@jonny-so
Copy link
Contributor Author

jonny-so commented Oct 5, 2024

Actually, the biased/unbiasedness refers to the autocovariance, not the autocorrelation. Maybe the parameter names need a rethink.

@fehiepsi
Copy link
Member

fehiepsi commented Oct 5, 2024

I think we should go with the biased version by default if the unbiased one is not practical.

Copy link
Member

@fehiepsi fehiepsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @jonny-so!

@fehiepsi fehiepsi merged commit e462292 into pyro-ppl:master Oct 6, 2024
4 checks passed
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

Successfully merging this pull request may close these issues.

2 participants