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

WIP: Add action(policy, s) interface to exploration policies #510

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johannes-fischer
Copy link
Contributor

To address #497, I extended EpsGreedyPolicy by internal fields for the greedy policy and the iteration k. If this seems like a reasonable way to go, I can make similar changes to SoftmaxPolicy and edit doc strings. If not feel free to close this PR.

With this change both interfaces are supported:
action(p::EpsGreedyPolicy, on_policy::Policy, k, s)
action(p::EpsGreedyPolicy, s)

@zsunberg
Copy link
Member

Thanks for the contribution!

My only concern is that it goes against the "preferably only one obvious way to do it" wisdom (https://peps.python.org/pep-0020/), since now k is in multiple places.

But I think this is probably OK. I think you should go ahead and implement it for the other policies and I will merge if it looks good.

@zsunberg
Copy link
Member

Is there a name we could use besides update!? Perhaps something more specific like set_age!

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