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

Only use effective number of samples if neff is explicitly specified. #51

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

kdolum
Copy link
Collaborator

@kdolum kdolum commented Jan 4, 2024

Don't compute or use effective number of samples unless neff is given.
Always import acor but continue silently if not found. In that case, if neff is set, call to acor fails because it is not there.
Fixes #50.

@kdolum
Copy link
Collaborator Author

kdolum commented Jan 4, 2024

I tested this using sample.py with and without acor (really encor) loaded and with and without supplying neff, and it seems to work. The autocorrelation length was extremely high, so maybe it is not being properly computed or maybe this test case is too simple in some way, but given the results from acor, the sampling process performed as it should.

@kdolum kdolum mentioned this pull request Jan 4, 2024
@kdolum
Copy link
Collaborator Author

kdolum commented Jan 6, 2024

I tested my changes in a simple enterprise data analysis. It works fine, in a certain sense. That is to say, if I supply neff it crashes on the first call to acor (from encor) with an error such as The autocorrelation time is too long relative to the variance in dimension -1893825599. But if I don't supply neff, then it works fine. Previously it gave this error regardless of whether I supplied neff. So this provides the feature that I wanted.
Someday, someone could fix acor, but I don't think we should hold up this change for that.

@kdolum kdolum requested a review from vhaasteren January 15, 2024 23:59
Copy link
Member

@vhaasteren vhaasteren left a comment

Choose a reason for hiding this comment

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

I went over all the changes. All this looks fine to me

@vhaasteren vhaasteren merged commit 9811073 into master Jan 23, 2024
10 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.

Remove dependence on acor
2 participants