search

Yelp / MOE

A global, black box optimization engine for real world metric optimization.
Other
1.31k stars 140 forks source link

Hook up BFGS to rest #352

Closed denizokt closed 10 years ago

denizokt commented 10 years ago

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

Reviewers: @suntzu86

***** DESCRIPTION ** Branch Name: denizokt_285_qei_rest Ticket(s)/Issue(s): Closes #285

***** TESTING DONE *****

denizokt commented 10 years ago

i think there are some merge conflicts here... @suntzu86 can you make sure?

denizokt commented 10 years ago

okay yeah schemas got messed up oops

suntzu86 commented 10 years ago

Couple of things:

  1. you never default to BFGS. is that intended?

  2. you should have at least done manual testing to see that BFGS was running for the appropriate POST request. (i.e., when you specify it manually and when it's the default behavior, if ever.) Did you? (this can be as ghetto as adding a print or log line to BFGS endpoint and as fancy as implementing the 'status' output for the multistart_optimizer).

    But it's important to verify that what you think is happening is actually happening. @sc932 is there an easy way to test this? i.e., for these endpoints with somewhat complex branching behavior to determine what exactly to do, it'd be nice to have tests that verify that we hit each code path when expected.

suntzu86 commented 10 years ago

hmmm see my most recent comment but we need to do something about cases where qEI fails. this might just be NaNs but I'm not sure.

denizokt commented 10 years ago

@suntzu86

  1. it was originally intended, but now i changed it to be smarter.
  2. yeah i did do manual testing for bfgs. it definitely seemed to return the same next point as gradient descent, up to 6-8 decimal places. i think we can also add unit tests to verify this.
sc932 commented 10 years ago

Looks good to me. After you fix what @suntzu86 suggested and travis passes I am happy to :ship:

suntzu86 commented 10 years ago

Hm, I guess make a ticket for these last comments.

It'd be better if mvndst num iterations, releps, etc. were settable. In the same way that the ExpectedImprovementEvaluator has a num_mc_iterations member, you could make a namedtuple for mvndst parameters so users can set this stuff. Esp if you say bfgs is really slow, some users may want less accurate computations. And we want tests to run quickly as well (unless the accuracy is needed for consistency as in your failing test).

How much slower was bumping maxiter by 10x and decreasing releps to 1.0e-9?

sc932 commented 10 years ago

@suntzu86 @bulutcocuk I vote for addressing these issues as a P1 followup ticket. I'm glad this is in master now, but we should make sure to clean up these last little bits.

suntzu86 commented 10 years ago

@sc932 @bulutcocuk cool, ticketing sounds good to me. These last few things aren't big changes anyway.

Also if it looks like I forgot about your review, it's probably b/c I did. Please do ping me if you've been waiting for feedback. I don't mean to be so slow :(