Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG+1] Fix [RandomizedSearchCV] Do not enforce that n_iter is less than or equal to size of search space #10982

Merged
merged 9 commits into from Apr 20, 2018

Conversation

Projects
None yet
3 participants
@julietcl
Copy link
Contributor

julietcl commented Apr 15, 2018

Reference Issues/PRs

Fixes #10900

What does this implement/fix? Explain your changes.

Changed exception raised if ParameterSampler is instantiated with a greater value of n_iter than the total space of parameters in the parameter grid to a warning. n_iter now acts as an upper bound.

Any other comments?

Juliet Lawton added some commits Apr 15, 2018

@jnothman
Copy link
Member

jnothman left a comment

Please change the tests to account for the new behaviour

'The total space of parameters %d is smaller '
'than n_iter=%d. For exhaustive searches, use '
'GridSearchCV.' % (grid_size, self.n_iter), RuntimeWarning)
self.n_iter = grid_size

This comment has been minimized.

Copy link
@jnothman

jnothman Apr 15, 2018

Member

Hmm. We usually try to avoid modifying the user-friendly provided parameters. Use a local variable instead

This comment has been minimized.

Copy link
@julietcl

julietcl Apr 15, 2018

Author Contributor

So something like this?

if all_lists:
            # look up sampled parameter settings in parameter grid
            param_grid = ParameterGrid(self.param_distributions)
            grid_size = len(param_grid)
            iterations = self.n_iter

            if grid_size < iterations:
                warnings.warn(
                    'The total space of parameters %d is smaller '
                    'than n_iter=%d. Running %d iterations. For exhaustive searches, '
                    'use GridSearchCV.' % (grid_size, self.n_iter, grid_size),
                     RuntimeWarning)
                iterations = grid_size
            for i in sample_without_replacement(grid_size, iterations,
                                                random_state=rnd):
                yield param_grid[i]

This comment has been minimized.

Copy link
@julietcl

julietcl Apr 16, 2018

Author Contributor

I will change iterations to n_iter.

warnings.warn(
'The total space of parameters %d is smaller '
'than n_iter=%d. For exhaustive searches, use '
'GridSearchCV.' % (grid_size, self.n_iter), RuntimeWarning)

This comment has been minimized.

Copy link
@jnothman

jnothman Apr 15, 2018

Member

I think you should state "running %d iterations" to clarify

@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Apr 16, 2018

@julietcl

This comment has been minimized.

Copy link
Contributor Author

julietcl commented Apr 16, 2018

For adding to the tests, I assume I should change assert_raises(ValueError, list, sampler) in test_parameters_sampler_replacement() from test_search.py to assert_warns_message(RuntimeWarning, expected_warning)?

Juliet Lawton added some commits Apr 16, 2018

Juliet Lawton
Juliet Lawton
Juliet Lawton
Juliet Lawton
Juliet Lawton
@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Apr 16, 2018

(perhaps this should be a UserWarning, not a Runtime warning)

Please add an entry to the change log at doc/whats_new/v0.20.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

@jnothman jnothman changed the title [MRG] Fix [RandomizedSearchCV] Do not enforce that n_iter is less than or equal to size of search space [MRG+1] Fix [RandomizedSearchCV] Do not enforce that n_iter is less than or equal to size of search space Apr 16, 2018

@TomDLT

This comment has been minimized.

Copy link
Member

TomDLT commented Apr 20, 2018

LGTM, thanks @julietcl !

@TomDLT TomDLT merged commit e070611 into scikit-learn:master Apr 20, 2018

8 checks passed

ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: python2 Your tests passed on CircleCI!
Details
ci/circleci: python3 Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 95.09%)
Details
codecov/project 95.09% (+<.01%) compared to ca436e7
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.