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

metrop_accept returns whether the sample was accepted #2058

Merged
merged 1 commit into from
Apr 21, 2017

Conversation

ColCarroll
Copy link
Member

With the metropolis acceptance step, we were always (except in HamiltonianMC) checking whether a proposal had been accepted just after returning. This makes metrop_accept return a tuple instead, and I think makes the code a little more readable.

@@ -6,7 +6,7 @@
from numpy.random import uniform
from enum import IntEnum, unique

__all__ = ['ArrayStep', 'ArrayStepShared', 'metrop_select', 'Competence']
__all__ = ('ArrayStep', 'ArrayStepShared', 'metrop_select', 'Competence')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this? I'm confused :)

Copy link
Contributor

@springcoil springcoil left a comment

Choose a reason for hiding this comment

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

The rest looks fine to me except I don't understand the brackets change.

@ColCarroll
Copy link
Member Author

Ah shoot, I can remove that, though I think it is generally "better". My linter was complaining that when __all__ is a list, it is mutable so it is harder to tell what functions are exported.

@springcoil
Copy link
Contributor

springcoil commented Apr 19, 2017 via email

@ColCarroll
Copy link
Member Author

I don't think it is a big deal either way. Actually, just found a surprisingly contentious issue in pydocstyle about it, with the conclusion being "don't worry about it, and most people use a list."
PyCQA/pydocstyle#62
I'll switch back to a list to maintain consistency, and update my pydocstyle!

@twiecki
Copy link
Member

twiecki commented Apr 19, 2017

@ColCarroll Nice refactor. Is acceptance already stored in the sampler stats?

@ColCarroll
Copy link
Member Author

ColCarroll commented Apr 19, 2017 via email

@twiecki
Copy link
Member

twiecki commented Apr 19, 2017

Can do that separately and merge once tests pass.

@ColCarroll
Copy link
Member Author

Do you want to add acceptance to the sampler stats in a separate PR before merging this? I think it stands on its own, but can do that.

@twiecki twiecki merged commit b2be720 into pymc-devs:master Apr 21, 2017
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.

3 participants