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

Added Bandit UCB1-tuned implementation #371

Merged
merged 8 commits into from
Aug 14, 2014
Merged

Conversation

norases
Copy link
Contributor

@norases norases commented Aug 12, 2014

********* PEOPLE *************
Primary reviewer: @sc932

Reviewers: @suntzu86

********* DESCRIPTION **************
Branch Name: norases_366_ucb1_tuned_bandit
Ticket(s)/Issue(s): #366

********* TESTING DONE *************
make test
make style-test


"""
self._win += arm.win
self._loss += arm.loss
self._total += arm.total
if self._variance is not None or arm.variance is not None:
raise ValueError('Cannot add arms when variance is not None!')
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a sentence to the end saying: "Please combine arms manually." so they know it is possible, we just aren't going to try to do it for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@sc932
Copy link
Contributor

sc932 commented Aug 12, 2014

A few very minor nits, but looks great.

Excellent job getting these all out so quickly!

Fix and ship :)

]

# See http://en.wikipedia.org/wiki/Bernoulli_distribution for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

You want to write

#: My decription
VARIABLE=value

sphinx won't autodoc this unless it has the colon. If you need multiple lines of description, then:

#: line 1
#: line 2
#: ...
VARIABLE=value

Could you change this in your constants file (and any other bandit-related places) for constants that you'd like autodoc'ed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@suntzu86
Copy link
Contributor

looks good. My comments are mostly style but I have one organizational concern about how we treat variance. @sc932 opinion appreciated, see above.

Also, pull master. Your test failure should've been fixed by @bulutcocuk's latest push.

@suntzu86
Copy link
Contributor

shipit

@sc932
Copy link
Contributor

sc932 commented Aug 13, 2014

🚢 ⛵ 🐐

norases added a commit that referenced this pull request Aug 14, 2014
Added Bandit UCB1-tuned implementation
@norases norases merged commit fbe06a7 into master Aug 14, 2014
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