-
Notifications
You must be signed in to change notification settings - Fork 54
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
Added Epsilion Greedy #219
base: master
Are you sure you want to change the base?
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.
Thanks for the PR @JoeHolt!
@@ -42,22 +42,28 @@ initExp: | |||
default: 0.5 | |||
optional: true | |||
|
|||
pT: |
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.
Why is this required? The YAML file is for algorithm inputs/outputs. pT
is internal to the algorithm.
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.
I could not get butler to work/save parameters unless the variable in the yaml. I am not sure of the 'correct' way to get butler working.
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.
Hm... that's not how butler
is supposed to work. It looks like this should work if this is removed beacuse pT
is initialized. If it doesn't, could you give a traceback?
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.
I went back and removed the arg in myApp.yaml and it continued to work as expected. I am not sure what I was doing at the time of writing that...
|
||
# Update t for next run | ||
newT = t + 1 | ||
butler.algorithms.set(key='pt', value=newT) |
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.
I think there's a typo here (pt
vs pT
). But maybe we could provide a longer name, maybe answers_received
? That'd be a little easier to maintain.
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.
I agree, a more descriptive name would be better.
Added basic epsilion greedy to CardinalBandits