[MRG] Cleanup skopt/parameter.py with docs and some minor changes #75

MechCoder · 2016-06-03T07:24:48Z

Added docs and some minor cosmetics.
Removed prior for now so that we can compare our results with a randomized search where we have some prior knowledge about the candidates. (I understand it might be useful but YAGNI)

MechCoder · 2016-06-03T07:31:41Z

@AlexanderFabisch @fabianp Since you have used the library to an extent, we would like to hear your thoughts on this. The idea is to support function calls like this at some point of time

gp_optimize(func, [Real(2, 4), ["red", "green"]])

bounds will be either a list of Distribution objects or lists. If it is a list of lists, then the type of parameter will be inferred.

(I am just cleaning up the work of @betatim done here #70)

AlexanderFabisch · 2016-06-03T08:51:02Z

That's a good idea. Here are some suggestions:

Maybe the argument name bounds is a little bit misleading for [Real(2, 4), ["red", "green"]]. You could introduce an additional argument param_distributions.
Real should actually be UniformReal. There could be NormalReal or LogReal as well like (similar to hyperopt).

However, my main focus at the moment is the optimization of real vectors so I actually don't really care about this use case.

AlexanderFabisch · 2016-06-03T08:58:12Z

By the way, is it possible to set a seed in scipy.stats distributions? It would be much better if you could simply pass a seed for the random number generator in the function call like in sklearn.

betatim · 2016-06-03T09:32:51Z

I would vote to keep it Real (hahaha...) and support non uniform priors via Real(..., prior=dist.norm(..)) or Real(..., prior=dist.exp(..)) etc. At least that was my thinking about having a prior parameter. What do you think?

To me it seems worth having the prior and the transform separate. The prior represents where you think likely good values are and the transform takes care of turning the values into something that is "ncie" for the optimiser to handle (onehot encoding, similar ranges)

fabianp · 2016-06-03T15:15:36Z

Hi, thanks for the heads up.

I'm general I'm skeptical for the need of such framework (but I could be wrong). In the usercases I'm familiar with you never need more than to set bounds and/or optionally transform to logspace, so this might be overkill. I know RandomizedSearchCV takes a distribution argument, but I've never used it for anything else besides uniform sampling (sometimes in log space).

MechCoder · 2016-06-04T06:16:05Z

Thanks a lot for your feedback.

@fabianp By skeptical, are you skeptical about the use of priors or the support for categorical parameters? A common use case would be the type of scaling used or the kernel in SVM (I remember the paper on randomized search had something on those lines).

I also think it would be okay to support priors via arguments later.

@AlexanderFabisch Yes, we would pass a random seed to the function call which will then be passed internally to the rvs method and then sampled (as being done currently at https://github.com/scikit-optimize/scikit-optimize/blob/master/skopt/gp_opt.py#L121). Were you meaning this or something else?

MechCoder · 2016-06-04T06:16:59Z

@betatim Please review and if you are happy, do merge. In follow-up PR's I can address the remaining.

AlexanderFabisch · 2016-06-04T21:17:59Z

@AlexanderFabisch Yes, we would pass a random seed to the function call which will then be passed internally to the rvs method and then sampled (as being done currently at https://github.com/scikit-optimize/scikit-optimize/blob/master/skopt/gp_opt.py#L121). Were you meaning this or something else?

That is what I meant.

glouppe · 2016-06-06T09:00:09Z

+1 for decoupling the domain/type of a variable (i.e., real, integer, categorical) from how it should be converted before being fed to the optimizer.

Overall, things should be very simple by default, e.g Real(2, 4)) (no transform, uniform distribution), but extendable in a plug and play manner with optional arguments, e.g. Real(2, 4, transformer="log", prior="gaussian").

glouppe · 2016-06-06T09:01:59Z

Also I agree that we should start by having something very simple to begin with, i.e. supporting real integer and categorical types, all assumed to be uniformly distributed, with an optional log transform for the real values. This should cover 80-90% of the user cases.

glouppe · 2016-06-06T15:55:49Z

Is this ready for review? I am confused as this PR is adding parameters.py while parameter.py (without an s) is already there.

glouppe · 2016-06-06T16:00:36Z

Could you instead base your PR on master? It is otherwise difficult to compare what you are actually changing. Not sure what was wrong with Tim's proposal. (Seems fine with me)

glouppe · 2016-06-07T07:38:01Z

skopt/parameter.py

@@ -151,6 +178,8 @@ def __init__(self, low, high, prior='uniform', transformer='identity'):

        if transformer == 'identity':
            self.transformer = Identity()
+        elif transformer == 'log':
+            self.transformer = Log()


Does it make sense to allow log transform for integers? Not sure we should support it, or at least the inverse transform should be cast back to integers.

Maybe not or at least I can't think of a quick use case.

MechCoder · 2016-06-07T07:41:04Z

There are still tests to be added.

MechCoder · 2016-06-07T07:49:01Z

Checkout the bug in the last commit :P

betatim · 2016-06-07T08:51:34Z

Woah! Thanks for catching this.

betatim · 2016-06-07T08:56:31Z

skopt/parameter.py

+    @abc.abstractmethod
+    def rvs(self, n_samples=None, random_state=None):
+        """
+        Sample points randomly.


Change to "Randomly sample points from the original space" to explain that the samples are not in the warped space.

MechCoder · 2016-06-08T07:08:48Z

Great, I'll change it then.

betatim · 2016-06-08T08:47:55Z

My thinking behind splitting the prior and transformer was that there are two problems that need solving:

how to transform values that humans like (eg strings to label categories) to values that optimisers like (eg one hot encoded categories)
express how I believe the most likely/useful values of a parameter are distributed (eg gaussian around 5123 with std of 42)

Personally I find it easier to think about the prior in the original space and then let the computer transform it to the warped space for the optimiser: gauss(5123, std=42) plus StandardScaler() to get it to a sensible range for the optimiser. Maybe this is overkill/just my weird brain :)

MechCoder · 2016-06-08T15:25:38Z

Yes, which is why it makes sense to keep sampling in the warped space just for the uniform prior.

glouppe · 2016-06-08T16:07:21Z

Proposal:

We hide the transformation mechanism. This is something internal and users should not deal with it.
We define two hardcoded priors for now, "uniform" and "log-uniform". In the later case, everything is done internally so that the optimizer deal with values in log-space, and then converted back to the original space.

MechCoder · 2016-06-09T07:21:52Z

@glouppe @betatim I've done as you have suggested. The tests should be the best way to understand the new API.

MechCoder · 2016-06-09T07:23:03Z

skopt/parameter.py

 from sklearn.preprocessing import LabelEncoder, OneHotEncoder
 from sklearn.utils import check_random_state
 from sklearn.utils.fixes import sp_version


-class Identity(TransformerMixin):


Removed because TransformerMixin assumes input to transform to be 2-D

MechCoder · 2016-06-09T07:24:38Z

I have tried to keep the API as simple as possible.

fabianp · 2016-06-09T07:55:28Z

I like where this is going :-)
On Jun 9, 2016 9:24 AM, "Manoj Kumar" notifications@github.com wrote:

I have tried to keep the API as simple as possible.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#75 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAQ8h5sitgMXxrZgWXcFVka-WQtyYxiiks5qJ7-3gaJpZM4ItRTO
.

glouppe · 2016-06-09T08:00:51Z

skopt/parameter.py


-    def transform(self, values):
-        return np.log(values)
+    @abc.abstractmethod


Do we really need abc and stuff? A simple raise NotImplementedError wouldnt be enough?

(This is an open question, I have no strong opinion against one or the other.)

I am -0 on abc

The difference is that making it an abc will prevent the instance being created if it does not implement those methods, while raising a NotImplementedError will allow the instance being created.

(I am not a purist and I don't mind removing them)

glouppe · 2016-06-09T08:14:06Z

Thanks Manoj, this is looking good!

I think we should add a last test showcasing how non-trivial grids can be defined. E.g., by calling sample_points for grids defined in different but equivalent formats (e.g. Real(0, 1) and (0., 1.)) and checking that for the same random_states, the same points are yielded.

glouppe · 2016-06-09T08:17:11Z

For ease of use, we should also accept triples for Real, where third argument is the prior string. That is:

(0., 1.) -> Real(0., 1., prior="uniform")
(0., 1., "log-uniform") -> Real(0., 1., prior="log-uniform")

betatim · 2016-06-09T13:51:00Z

skopt/parameter.py

-        return self._rvs.rvs(size=n_samples, random_state=random_state)
+        random_vals = self._rvs.rvs(size=n_samples, random_state=random_state)
+        if self.prior == "log-uniform":
+            return np.exp(random_vals)


np.exp(x) -> 10**x to match the np.log10 done earlier.

MechCoder · 2016-06-10T07:36:54Z

All right, fixed both the inverse_transform and the tests. We can introduce the tuple shorthand in another PR. Please merge if happy

glouppe · 2016-06-10T07:43:30Z

This is great, thank you! Waiting for Travis green light, and then I'll merge.

glouppe · 2016-06-10T07:44:05Z

I'll address the shorthand.

MechCoder · 2016-06-10T07:58:01Z

Yay! We should also refactor the existing minimize code to make use of the new API.

glouppe · 2016-06-10T08:20:47Z

I just made #81 with a few thrown ideas.

betatim · 2016-06-10T12:16:48Z

Whoop!

Cleanup Real with docs and some minor changes

ed7913b

MechCoder force-pushed the cleanup branch 2 times, most recently from 8eaf198 to 544ef4f Compare June 7, 2016 07:36

glouppe reviewed Jun 7, 2016
View reviewed changes

MechCoder force-pushed the cleanup branch from 544ef4f to 394211c Compare June 7, 2016 07:38

cleanup

0cda21d

MechCoder force-pushed the cleanup branch from 394211c to 0cda21d Compare June 7, 2016 07:40

MechCoder changed the title ~~Cleanup Real with docs and some minor changes~~ Cleanup skopt/parameter.py with docs and some minor changes Jun 7, 2016

Fix bug in Real

e08073d

betatim reviewed Jun 7, 2016
View reviewed changes

MechCoder added 3 commits June 8, 2016 23:17

Start refactoring tests

c39e66f

Refactor Real and add tests

0a367cd

Refactor Categorical

6e81063

MechCoder changed the title ~~[WIP] Cleanup skopt/parameter.py with docs and some minor changes~~ [MRG] Cleanup skopt/parameter.py with docs and some minor changes Jun 9, 2016

MechCoder reviewed Jun 9, 2016
View reviewed changes

glouppe reviewed Jun 9, 2016
View reviewed changes

betatim reviewed Jun 9, 2016
View reviewed changes

Add support for inverse_transform of CategoricalEncoder

c78e0c7

Add tests for consistency

4429019

MechCoder force-pushed the cleanup branch from 4351de3 to 4429019 Compare June 10, 2016 07:40

glouppe merged commit ce9b9bc into scikit-optimize:master Jun 10, 2016

MechCoder deleted the cleanup branch June 10, 2016 07:58

glouppe mentioned this pull request Jun 10, 2016

Refactor minimize functions to make use of sampling API #81

Closed

[MRG] Cleanup skopt/parameter.py with docs and some minor changes #75

[MRG] Cleanup skopt/parameter.py with docs and some minor changes #75

Conversation

MechCoder commented Jun 3, 2016

MechCoder commented Jun 3, 2016

AlexanderFabisch commented Jun 3, 2016

AlexanderFabisch commented Jun 3, 2016

betatim commented Jun 3, 2016

fabianp commented Jun 3, 2016

MechCoder commented Jun 4, 2016

MechCoder commented Jun 4, 2016

AlexanderFabisch commented Jun 4, 2016

glouppe commented Jun 6, 2016

glouppe commented Jun 6, 2016 • edited

glouppe commented Jun 6, 2016 • edited

glouppe commented Jun 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MechCoder commented Jun 7, 2016

MechCoder commented Jun 7, 2016

betatim commented Jun 7, 2016

Choose a reason for hiding this comment

MechCoder commented Jun 8, 2016

betatim commented Jun 8, 2016

MechCoder commented Jun 8, 2016

glouppe commented Jun 8, 2016

MechCoder commented Jun 9, 2016

Choose a reason for hiding this comment

MechCoder commented Jun 9, 2016

fabianp commented Jun 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glouppe commented Jun 9, 2016 • edited

glouppe commented Jun 9, 2016 • edited

Choose a reason for hiding this comment

MechCoder commented Jun 10, 2016

glouppe commented Jun 10, 2016

glouppe commented Jun 10, 2016

MechCoder commented Jun 10, 2016

glouppe commented Jun 10, 2016

betatim commented Jun 10, 2016

glouppe commented Jun 6, 2016 •

edited

glouppe commented Jun 6, 2016 •

edited

glouppe commented Jun 6, 2016 •

edited

glouppe commented Jun 9, 2016 •

edited

glouppe commented Jun 9, 2016 •

edited