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] Support of the collections.Sequence type has been added to the _check_param_grid method from model_selection. #7323

Merged
merged 7 commits into from Sep 7, 2016

Conversation

Projects
None yet
4 participants
@b0noI
Contributor

b0noI commented Sep 1, 2016

Support of the collections.Sequence type has been added to the _check_param_grid method from model_selection.

Reference Issue

Fixes #7322

What does this implement/fix? Explain your changes.

_check_param_grid used to support only tuples/lists, now it supports any collection.Sequence.

Any other comments?

PR also includes small refactoring of the ValueError tests.

Support of the collections.Sequence type has been added to the _check…
…_param_grid method from model_selection.

@b0noI b0noI changed the title from Support of the collections.Sequence type has been added to the _check_param_grid method from model_selection. to [MRG ]Support of the collections.Sequence type has been added to the _check_param_grid method from model_selection. Sep 1, 2016

old_stdout = sys.stdout
sys.stdout = StringIO()
grid_search.fit(X, y)
sys.stdout = old_stdout

This comment has been minimized.

@ogrisel

ogrisel Sep 1, 2016

Member

I don't understand why you set verbose=3 in the first place. How is that related to tie handling?

This comment has been minimized.

@b0noI

b0noI Sep 2, 2016

Contributor

You are right for the sake of the test it's unnecessary, test has been refactored. Thank you.

test_grid_search_when_param_grid_includes_range test was refactored (…
…parts that are not nessesary have been removed).
@@ -332,7 +332,7 @@ def _check_param_grid(param_grid):
if isinstance(v, np.ndarray) and v.ndim > 1:
raise ValueError("Parameter array should be one-dimensional.")
check = [isinstance(v, k) for k in (list, tuple, np.ndarray)]
check = [isinstance(v, k) for k in (np.ndarray, Sequence)]

This comment has been minimized.

@jnothman

jnothman Sep 2, 2016

Member

I don't think we want to allow strings here. Strings are sequences.

Also, isinstance allows its second param to be a tuple of types, so if isinstance(v, (np.ndarray, Sequence)) would suffice here... What we want is something like if isinstance(v, (np.ndarray, Sequence)) and not isinstance(v, six.string_types). Please add a test to check that str raises an error.

This comment has been minimized.

@b0noI

b0noI Sep 3, 2016

Contributor

done

test_grid_search_bad_param_grid now checks that value is not string. …
…This is important since string is a Sequence.
if isinstance(v, six.string_types):
raise ValueError("Parameter values for parameter ({0}) should"
" not be a string.".format(name))

This comment has been minimized.

@jnothman

jnothman Sep 3, 2016

Member

I think it's okay to say "need to be a sequence (not a string)"

This comment has been minimized.

@jnothman

jnothman Sep 3, 2016

Member

or even without "(not a string)" and including it in the clause above.

This comment has been minimized.

@b0noI

b0noI Sep 3, 2016

Contributor

done.

@jnothman

This comment has been minimized.

Member

jnothman commented Sep 3, 2016

Please add a bug fix entry in whats_new.rst

@b0noI

This comment has been minimized.

Contributor

b0noI commented Sep 3, 2016

whats_new.rst has been updated.

- :func: `model_selection.tests._search._check_param_grid` now works correctly with all types
that extended from sequence (except string), including range (Python 3.x) and xrange
(Python 2.x).
(`#7323 <https://github.com/scikit-learn/scikit-learn/pull/7323>`_) by `Viacheslav Kovalevskyi`_.
- :class:`StratifiedKFold` now raises error if all n_labels for individual classes is less than n_folds.

This comment has been minimized.

@jnothman

jnothman Sep 3, 2016

Member

usually we leave a space between entries

@@ -295,6 +295,10 @@ Enhancements
Bug fixes
.........
- :func: `model_selection.tests._search._check_param_grid` now works correctly with all types
that extended from sequence (except string), including range (Python 3.x) and xrange

This comment has been minimized.

@jnothman

jnothman Sep 3, 2016

Member

Should be Sequence really (and "extended from" isn't precise, because any class can declare itself as implementing Sequence without extending anything)

@jnothman

This comment has been minimized.

Member

jnothman commented Sep 3, 2016

LGTM

@jnothman jnothman changed the title from [MRG ]Support of the collections.Sequence type has been added to the _check_param_grid method from model_selection. to [MRG+1] Support of the collections.Sequence type has been added to the _check_param_grid method from model_selection. Sep 3, 2016

@b0noI

This comment has been minimized.

Contributor

b0noI commented Sep 7, 2016

ogrisel@ please let me know if there are any other changes that you want me to do in the PR, and I would be happy to do them.

@amueller

This comment has been minimized.

Member

amueller commented Sep 7, 2016

lgtm.

@amueller amueller merged commit a03db89 into scikit-learn:master Sep 7, 2016

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

rsmith54 pushed a commit to rsmith54/scikit-learn that referenced this pull request Sep 14, 2016

[MRG+1] Support of the collections.Sequence type has been added to th…
…e _check_param_grid method from model_selection. (scikit-learn#7323)

* Support of the collections.Sequence type has been added to the _check_param_grid method from model_selection.

* test_grid_search_when_param_grid_includes_range test was refactored (parts that are not nessesary have been removed).

* test_grid_search_bad_param_grid now checks that value is not string. This is important since string is a Sequence.

* _check_param_grid now checks is the type is not string together with the check for other types.

* whats_new.rst has been updated to include information about bug fix for bug scikit-learn#7322.

* Description of the fix for the bug scikit-learn#7322 has been updated.

* Fix for indented in model_selection._search.py.

TomDLT added a commit to TomDLT/scikit-learn that referenced this pull request Oct 3, 2016

[MRG+1] Support of the collections.Sequence type has been added to th…
…e _check_param_grid method from model_selection. (scikit-learn#7323)

* Support of the collections.Sequence type has been added to the _check_param_grid method from model_selection.

* test_grid_search_when_param_grid_includes_range test was refactored (parts that are not nessesary have been removed).

* test_grid_search_bad_param_grid now checks that value is not string. This is important since string is a Sequence.

* _check_param_grid now checks is the type is not string together with the check for other types.

* whats_new.rst has been updated to include information about bug fix for bug scikit-learn#7322.

* Description of the fix for the bug scikit-learn#7322 has been updated.

* Fix for indented in model_selection._search.py.

paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

[MRG+1] Support of the collections.Sequence type has been added to th…
…e _check_param_grid method from model_selection. (scikit-learn#7323)

* Support of the collections.Sequence type has been added to the _check_param_grid method from model_selection.

* test_grid_search_when_param_grid_includes_range test was refactored (parts that are not nessesary have been removed).

* test_grid_search_bad_param_grid now checks that value is not string. This is important since string is a Sequence.

* _check_param_grid now checks is the type is not string together with the check for other types.

* whats_new.rst has been updated to include information about bug fix for bug scikit-learn#7322.

* Description of the fix for the bug scikit-learn#7322 has been updated.

* Fix for indented in model_selection._search.py.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment