Skip to content

WIP: First draft of a random search in GridSearchCV#455

Closed
alextp wants to merge 1 commit intoscikit-learn:masterfrom
alextp:randomized-cv
Closed

WIP: First draft of a random search in GridSearchCV#455
alextp wants to merge 1 commit intoscikit-learn:masterfrom
alextp:randomized-cv

Conversation

@alextp
Copy link
Copy Markdown
Member

@alextp alextp commented Dec 5, 2011

Hi,

So far, no tests, so this is just to get feedback on the API and goals of an extension to sklearn's GridSearchCV that does random search when given a budget.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In order to have reproducible results, we should instead use a random_state (added and initialized in the constructor), and then call shuffle on it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes but in the following way:

from random import Random
from sklearn.utils import check_random_state
...

    # in __init__
    self.random_state = random_state

...

    # then in fit
    self.random_state = check_random_state(self.random_state)

    if self.budget:
        py_random_state = Random(self.random_random.rand())
        self.rolled_out_grid = list(IterGrid(param_grid))
        py_random_state.shuffle(self.rolled_out_grid)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the purpose of py_random_state? Not to change the estimator during fit? I would do
random_state = check_random_state(self.random_state) and then use random_state later. Or doesn't check_random_state make a copy? Maybe it should.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

because AFAIK the numpy rng cannot shuffle inplace a python list.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, that was the subtlety I knew I overlooked, thanks :)

@amueller
Copy link
Copy Markdown
Member

amueller commented Mar 3, 2013

Closing this one as I just merged #1194.

@amueller amueller closed this Mar 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants