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

Implement Epsilon-First bandit endpoint #350

Merged
merged 9 commits into from
Aug 1, 2014

Conversation

norases
Copy link
Contributor

@norases norases commented Aug 1, 2014

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

Reviewers: @suntzu86

********* DESCRIPTION **************
Branch Name: norases_335_create_epsilon_first_bandit_endpoint
Ticket(s)/Issue(s): Closes #335

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

A class to encapsulate the computation of bandit epsilon first.

total_samples is the total number of samples (#to sample + #sampled)
#sampled is calculated by summing up total from each arm sampled.
Copy link
Contributor

Choose a reason for hiding this comment

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

lets spell out the word number, people may think this is a hashtag or something :p

maybe a link to wikipedia too (you deleted that above)

Copy link
Contributor

Choose a reason for hiding this comment

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

nm, I see the link below. s/#/number/g still though

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 1, 2014

Looks really good. Great docs. Good handling of multiple schemas. Good tests.

Just a few random style nits.

Since we are on trail #41, we are in the exploration phase. We choose arms randomly:

arm1: 0.33, arm2: 0.33, arm3: 0.33.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you run make -B docs and looked over your rendered doc pages? You should do this.

My experience has been that math, dicts, pseudocode, etc. all read more nicely if wrapped in at least double backticks if not at math-block, e.g.,

the thing, ``{win:10, lose:10, ...}`` is cool

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

Implementers of this interface will never override this method.

:return: of set of names of the winning arms
:rtype: frozenset(String())
Copy link
Contributor

Choose a reason for hiding this comment

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

:param: and :type: for input

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

suntzu86 commented Aug 1, 2014

looks good besides some comment fixes. not sure why your travis failed?

also, can you update the changelog?

@sc932 thoughts on my comment above? <-- nevermind, my comment was misinformed

@norases
Copy link
Contributor Author

norases commented Aug 1, 2014

@suntzu86 I have updated the changelog. I will let @sc932 comment on the hack issue.

:param arms_sampled: a dictionary of arm name to :class:`moe.bandit.data_containers.SampleArm`
:type arms_sampled: dictionary of (String(), SampleArm()) pairs
:return: of set of names of the winning arms
:rtype: frozenset(String())
Copy link
Contributor

Choose a reason for hiding this comment

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

add a :raise: ValueError when ... after rtype

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

@suntzu86
Copy link
Contributor

suntzu86 commented Aug 1, 2014

one more test and shipit!

@sc932
Copy link
Contributor

sc932 commented Aug 1, 2014

👞 it!

@suntzu86
Copy link
Contributor

suntzu86 commented Aug 1, 2014

shipit!

norases added a commit that referenced this pull request Aug 1, 2014
…ndit_endpoint

Implement Epsilon-First bandit endpoint
@norases norases merged commit 902b21e into master Aug 1, 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.

Create Epsilon-First Bandit endpoint
3 participants