[MRG] Model expected inverse time along with function #369

MechCoder · 2017-04-26T05:54:18Z

Add tests
Support return_grad=True when per_second=True

codecov-io · 2017-04-26T06:27:05Z

Codecov Report

Merging #369 into master will increase coverage by 0.02%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master     #369      +/-   ##
==========================================
+ Coverage   86.22%   86.25%   +0.02%     
==========================================
  Files          22       22              
  Lines        1496     1550      +54     
==========================================
+ Hits         1290     1337      +47     
- Misses        206      213       +7

Impacted Files	Coverage Δ
skopt/optimizer/gp.py	`100% <ø> (ø)`	⬆️
skopt/optimizer/gbrt.py	`100% <ø> (ø)`	⬆️
skopt/optimizer/base.py	`94.82% <ø> (+1.27%)`	⬆️
skopt/optimizer/forest.py	`100% <ø> (ø)`	⬆️
skopt/utils.py	`98.33% <100%> (+0.23%)`	⬆️
skopt/acquisition.py	`96.84% <100%> (+0.78%)`	⬆️
skopt/optimizer/optimizer.py	`93.61% <81.08%> (-3.95%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c8bbd0...9dd249d. Read the comment docs.

betatim · 2017-04-27T20:47:27Z

edit: moved to the issue thread, keep the PR to technical discussions

glouppe · 2017-05-01T08:13:48Z

skopt/optimizer/base.py

+
+        This implies `1/t` follows a lognormal distribution with mean `-m`
+        and `sigma` or `E(1/t) = exp(-m + sigma**2)`
+        The next point suggested is the point that minimizes `acq(x)*E(-1/t(x))`


Just curious, do you have a reference for this strategy?

A smart strategy, but which would require quite some changes in our codebase, is freeze-thaw bayesian optimization (http://arxiv.org/abs/1406.3896) where the idea is basically to extrapolate the function wrt some its parameters (like n_epochs). This is in turn allows to do early stopping, thereby reducing the total execution time.

Yes, see section 3.2 of https://arxiv.org/pdf/1206.2944.pdf .

I'll have to look at the other paper though.

glouppe · 2017-05-02T06:26:52Z

While in general I am in favor of taking running time into account, I also think we should be cautious in what we end up implementing. Most of those approaches are very state-of-the-art -- that is none are well-established. So before adding yet another option to the minimizers, I think it would be wise to really establish first whether this is worth it.

MechCoder · 2017-06-01T06:58:01Z

I removed the include_time parameter and added support for acquisition functions based on the suffix "ps". I still need to finish manually compute gradients for optimizations.

I'll add tests and some experiments in a while.

MechCoder · 2017-06-01T16:16:47Z

Can someone make a pass for correctness?

MechCoder · 2017-06-02T05:56:31Z

I wrote a toy example here, the function optimized is a constant function while the time is proportional to the input. Here are the "EI" and "EIps" values after 3 points have been seen.

def contant_func(x):
    return 10.0

def time_func(x):
    """
    Hard-code such that time is x[0]
    """
    return contant_func(x), log(x[0])

x = [3, 5, 7]
x_1D = np.reshape(x, (-1, 1))
y = [contant_func(xi) for xi in x]
x_pred = np.linspace(1, 9, 100)
x_pred1D = np.reshape(x_pred, (-1, 1))

# Naive EI.
gpr = GaussianProcessRegressor(random_state=0)
gpr.fit(x_1D, y)
y_pred = gpr.predict(x_pred1D)
ei_vals = gaussian_ei(x_pred1D, gpr, y_opt=10.0)

# EI that takes into account time.
y_2D = [time_func(xi) for xi in x_1D]
mor = MultiOutputRegressor(gpr)
mor.fit(x_1D, y_2D)
eips_vals = gaussian_ei(x_pred1D, mor, y_opt=10.0, per_second=True)

plt.plot(x, y, "ro")
plt.plot(x_pred, y_pred, label="mean")
plt.plot(x_pred, ei_vals, label="EI")
plt.plot(x_pred, eips_vals, label="EIps")
plt.legend()
plt.show()

betatim · 2017-06-02T06:07:54Z

I can take a closer look maybe this afternoon (EU time) or next week. Had a quick look now and was wondering if we should make the infrastructure to support "per second" already flexible enough to handle arbitrary multi-objective problems? I think that would mainly influence decision in Optimizer, the user facing parts would stay the same. Thoughts?

MechCoder · 2017-06-02T06:18:14Z

We can do that but I don't think the present acquisition function machinery can handle multitask problems. I'd prefer writing separate acquisition functions as and when we decide to support that.

MechCoder · 2017-06-02T09:16:05Z

Updated to [MRG]. Ready for review from my side.

MechCoder · 2017-06-12T13:30:02Z

Can I attract some attention here?

betatim · 2017-06-13T06:56:28Z

I don't really know much about the science side of EI per second etc. Would be good to have @glouppe comment on that, at least he seems to know that none are established methods??

MechCoder · 2017-06-19T06:48:45Z

Since @glouppe 's initial comment, I have moved the option to include time as an option to the minimizers to a new acquisition function. Not sure what you mean by "the science side of EI", can you please elaborate? Also, surely my links to the original bayesian optimisation paper and the matlab toolbox that allows such options make it a more established method than gbrt_minimize which our codebase supports which hasn't been published?

MechCoder · 2017-06-19T07:02:15Z

In the graph (#369 (comment)), you can see a constant function being modelled by the GP, with time proportional to the logarithm of x. Including the time, makes sure that if there are two points to the left and right of the origin, it is likelier to pick the point on the left.

You are right that one cannot fine-tune the balance between the function and time, but I would prefer implementing methods that are already there rather than something new as a starting point.

MechCoder · 2017-06-23T05:22:18Z

@iaroslav-ai I've removed references to LCBps and gphedgeps

iaroslav-ai · 2017-06-27T06:21:51Z

skopt/optimizer/base.py

        - `"EI"` for negative expected improvement,
        - `"PI"` for negative probability of improvement.
+        - ``"EIps"`` for negated expected improvement per second to take into
+          account the function compute time. Then, the objective function is


Nitpick: maybe drop "Then, ", seems a bit redundant

iaroslav-ai · 2017-06-27T06:24:25Z

skopt/optimizer/base.py

+        else:
+            if np.ndim(y0) != 2 and np.shape(y0)[1] != 2:
+                raise ValueError(
+                    "`y0` elements should be a tuple of 2 values.")


Nitpick: one might get confused by this message by assuming that y0 as such should be a tuple of 2 values. Maybe use something like "Every element in y0 should be a tuple or list containing 2 scalar values"

iaroslav-ai · 2017-06-27T06:28:07Z

skopt/acquisition.py

+            acq_vals = -func_and_grad

    else:
        raise ValueError("Acquisition function not implemented.")


Consider maybe adding here also some code which prints the list of supported acquisition function identifiers. Might be useful for someone who is familiar with BO but not with skopt, or for someone lazy who does not want to look up how to spell the acquisition name properly.

I have added detailed error messages in the Optimizer instance. It's unlikely that anyone will use this private function directly.

iaroslav-ai · 2017-06-27T06:50:06Z

skopt/tests/test_acquisition.py

        return np.zeros(X.shape[0]), np.ones(X.shape[0])


+class MultiOutputSurrogate:


Its more like a two output surrogate, we might want to add multi - output surrogates later, might be better to spare this name for now to use it later. Could you use something along the lines of "TwoOutputSurrogate"?

iaroslav-ai · 2017-06-27T06:55:56Z

skopt/tests/test_acquisition.py

+    X_pred =  [[1], [2], [4], [6], [8], [9]]
+    for acq_func in ["EIps", "PIps"]:
+        vals = _gaussian_acquisition(X_pred, mos, y_opt=1.0, acq_func=acq_func)
+        for fast, slow in zip([0, 1, 2], [5, 4, 3]):


Could you here just check if vals is a sorted list? Something like

all(vals[i] <= vals[i+1] for i in range(len(vals)-1))

Or maybe skip some elements to be sure

all(vals[i] <= vals[i+2] for i in range(len(vals)-2))

... to make it a bit more easy to pass the test for the surrogate - while I would expect that most of the time it should learn the right thing, it might not just because data is not too much

iaroslav-ai · 2017-06-27T06:59:52Z

skopt/tests/test_common.py

 MINIMIZERS = [gp_minimize]
 ACQUISITION = ["LCB", "PI", "EI"]
-
+ACQ_FUNCS_PS = ["LCBps", "PIps", "EIps", "gp_hedgeps"]


Don't forget to remove here the unsupported acquisitions

iaroslav-ai · 2017-06-27T07:09:26Z

Just thinking out loud: a user one can always have a tradeoff between value of time and objective in principle by just multiplying the time returned in the objective function by some constant. This would be good to clarify in documentation somewhere. Still explicit tradeoff parameter would be better I think.

MechCoder · 2017-06-28T12:02:33Z

skopt/optimizer/base.py

        if len(x0) != len(y0):
            raise ValueError("`x0` and `y0` should have the same length")

-        if not all(map(np.isscalar, y0)):


these checks were moved inside Optimizer

MechCoder · 2017-06-28T12:05:04Z

@iaroslav-ai I've addressed all your comments and have added support for gradient with gradient checks. I would postpone adding an example and allowing the user to configure the tradeoff to another pull request. Please have another look.

iaroslav-ai · 2017-07-01T10:37:52Z

skopt/optimizer/forest.py

+          account the function compute time. Then, the objective function is
+          assumed to return two values, the first being the objective value and
+          the second being the time taken.
+        - `"PIps"` for negated probability of improvement per second.


Consider adding here that the objective function is assumed to be the same as for 'EIps', or maybe describe "EIps" and "PIps" jointly. Otherwise someone who might be in a rush or tired can just miss the implied similarity of objective function as is for the 'EIps', which might lead to some confusion.

iaroslav-ai · 2017-07-01T10:46:10Z

skopt/optimizer/optimizer.py

+        if "ps" in self.acq_func:
+            if is_2Dlistlike(x):
+                if np.ndim(y) == 2 and np.shape(y)[1] == 2:
+                    y = [[val, log(t)] for (val, t) in y]


A bit of a nitpick: maybe use log(t + 1e-3), for example in case user measures time in minutes, and then it might happen that for some task the time is zero minutes (< 60 seconds).

Maybe it is easier to document that time should be in seconds?

iaroslav-ai · 2017-07-01T10:52:49Z

skopt/tests/test_acquisition.py

        return np.zeros(X.shape[0]), np.ones(X.shape[0])


+class ConstantGPRSurrogate(object):


Add a comment somewhere around to explain why you need those.

iaroslav-ai · 2017-07-01T10:57:16Z

skopt/tests/test_acquisition.py

+
+@pytest.mark.fast_test
+@pytest.mark.parametrize("acq_func", ["EIps", "PIps"])
+def test_acquisition_per_second(acq_func):


You check here whether the acquisition function is working properly with an example surrogate ConstantGPRSurrogate by checking if the time function wrt x is learned, right? Would be good to have a comment elaborating on this

Added below but the diff view does not collapse it.

iaroslav-ai · 2017-07-01T11:01:47Z

LGTM when my latest comments are addressed!

MechCoder · 2017-07-10T00:22:33Z

Will merge when tests pass.

betatim mentioned this pull request Apr 26, 2017

Include expected compute time in the acquisition function #211

Closed

glouppe reviewed May 1, 2017

View reviewed changes

MechCoder force-pushed the acq_function_with_time branch 3 times, most recently from f06cafa to 4c00169 Compare May 31, 2017 14:45

Model expected inverse time along with function

7e2f965

MechCoder force-pushed the acq_function_with_time branch from c30b035 to 7e2f965 Compare June 1, 2017 06:37

MechCoder added 2 commits June 2, 2017 10:44

Add some documentation

e310bca

Fix EI

7d81c51

MechCoder force-pushed the acq_function_with_time branch from 68161b1 to 7d81c51 Compare June 2, 2017 05:54

MechCoder added 3 commits June 2, 2017 12:44

Start adding tests

1dd3ccf

test_optimizer_api

64e2333

Refactor

9c0a4fd

MechCoder changed the title ~~[WIP] Model expected inverse time along with function~~ [MRG] Model expected inverse time along with function Jun 2, 2017

MechCoder added 2 commits June 2, 2017 14:57

Add gp_hedegeps

952b602

More tests

2ab9ee8

MechCoder added this to the 0.4 milestone Jun 2, 2017

Remove lcbps and gphedgeps

114187d

iaroslav-ai reviewed Jun 27, 2017

View reviewed changes

Merge branch 'master' into acq_function_with_time

677008e

MechCoder force-pushed the acq_function_with_time branch from 4e146ce to 677008e Compare June 27, 2017 13:31

MechCoder added 5 commits June 28, 2017 01:33

Address some comments

c7b2f6b

Make acquisition function test stronger

c5fc9f8

Add gradient support for acquisition per second

8ec8afb

Remove checks from base.py

f6f5f74

Fix initial points

33d91f6

MechCoder commented Jun 28, 2017

View reviewed changes

iaroslav-ai reviewed Jul 1, 2017

View reviewed changes

iaroslav-ai added the New feature label Jul 1, 2017

MechCoder added 2 commits July 8, 2017 10:17

Merge branch 'master' into acq_function_with_time

7c9e31e

Final fixes and tests

9dd249d

MechCoder merged commit a151a9a into scikit-optimize:master Jul 10, 2017

MechCoder deleted the acq_function_with_time branch July 10, 2017 01:30

iaroslav-ai mentioned this pull request Jan 5, 2018

[MRG+2] Added support for named arguments with objective function (NEW version) #592

Merged

		return np.zeros(X.shape[0]), np.ones(X.shape[0])


		class MultiOutputSurrogate:

		return np.zeros(X.shape[0]), np.ones(X.shape[0])


		class ConstantGPRSurrogate(object):

[MRG] Model expected inverse time along with function #369

[MRG] Model expected inverse time along with function #369

Uh oh!

Conversation

MechCoder commented Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

betatim commented Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glouppe commented May 2, 2017

Uh oh!

MechCoder commented Jun 1, 2017

Uh oh!

MechCoder commented Jun 1, 2017

Uh oh!

MechCoder commented Jun 2, 2017

Uh oh!

betatim commented Jun 2, 2017

Uh oh!

MechCoder commented Jun 2, 2017

Uh oh!

MechCoder commented Jun 2, 2017

Uh oh!

MechCoder commented Jun 12, 2017

Uh oh!

betatim commented Jun 13, 2017

Uh oh!

MechCoder commented Jun 19, 2017

Uh oh!

MechCoder commented Jun 19, 2017

Uh oh!

MechCoder commented Jun 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iaroslav-ai commented Jun 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MechCoder commented Jun 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iaroslav-ai commented Jul 1, 2017

Uh oh!

MechCoder commented Jul 10, 2017

Uh oh!

MechCoder commented Apr 26, 2017 •

edited

Loading

codecov-io commented Apr 26, 2017 •

edited

Loading

betatim commented Apr 27, 2017 •

edited

Loading