Yelp / MOE

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

Pulled out mvndst parameters #381

Closed denizokt closed 10 years ago

denizokt commented 10 years ago

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

Reviewers: @suntzu86

***** DESCRIPTION ** Branch Name: denizokt_376_fixflakes Ticket(s)/Issue(s): Closes #376 Closes #374

edit (@suntzu86): this also closes the test performance issue in #374

***** TESTING DONE ***** make test make style-test made sure demos and pretty endpoints still work

suntzu86 commented 10 years ago

Some style things and a significant issue with how mvndst_parameters is stored in the EI class.

Thanks for getting this together quickly!

suntzu86 commented 10 years ago

One more thing... now that I read through where abseps and releps are used: http://www.math.wsu.edu/faculty/genz/software/fort77/mvndstpack.f and the default values in scipy:

Other Parameters
----------------
maxpts : input int, optional
    Default: 2000
abseps : input float, optional
    Default: 1e-06
releps : input float, optional
    Default: 1e-06

I'm actually a bit concerned/confused. mvndst says it uses eps = MAX(abseps, finest*releps). 1.0e-6 is pretty large absolute error (i think). That might mean these super low releps values we're setting don't actually do anything and only maxpts matters.

It'd be worth understanding what's happening here--maybe we should be using abseps or setting abseps to 0 so that only releps matters. Otherwise we're probably being overridden by abseps.

denizokt commented 10 years ago

okay i made abseps accessible as well.

now test_qd_with_self is running too slowly... the full test took 60 seconds to run with abseps=1.0e-11 and 14 for 1.0e-7 welp

1.0e-6 runs in < 3 seconds... good enough?

denizokt commented 10 years ago

@suntzu86 currently i don't actually access the private variable anywhere outside the expected improvement class. i was thinking that making getters/setters would be very redundant in this case. i could make them using the property(getter, setter) format but that would just make it have two copies of the same thing.

sc932 commented 10 years ago

Looks good. A few minor nits.

sc932 commented 10 years ago

Also, thanks again so much for getting this together so quickly :)

suntzu86 commented 10 years ago

@bulutcocuk fair enough, you can leave the getters/setters off then. I didn't realize all the private accesses outside the class were gone. still want the comment about what the different eps mean and how they relate to each other

suntzu86 commented 10 years ago

@bulutcocuk i don't understand what you're asking in that post about timing results. I think your numbers don't all refer to releps...? or something?

sc932 commented 10 years ago

:ship:

suntzu86 commented 10 years ago

shipit pending travis