[MRG+1] New text for the ValueError that is thrown by _check_param_grid method #7216
Conversation
…rameters name when values of a parameter have incorrect type. Old ValueError: ValueError: Parameter values should be a list. New ValueError: ValueError: Parameter(key1) values should be a non-empty list.
|
I don't mind something like this, but |
…rameters name when values of a parameter have incorrect type. Old ValueError: ValueError: Parameter values should be a list. New ValueError: ValueError: Parameter(key1) values should be a non-empty list.
|
jnothman@ done, both places have been updated. Thank you for the feedback, I didn't know about deprecation. PS: As soon as this one is merged, I will prepare PR that fixes support of range() in the method, since in Python3.X range does not return a list anymore. As a result some of the code is not working correctly after switching from Python2.X to 3.X |
I'm not sure what you're referring to... Can you link to the misuse of |
|
|
||
| if len(v) == 0: | ||
| raise ValueError("Parameter values should be a non-empty " | ||
| "list.") | ||
| raise ValueError("Parameter({0}) values should be a non-empty " |
There was a problem hiding this comment.
I'd rather "Parameter ({0}) value should be ..."
Simplified version of the logic from _check_param_grid method: check = [isinstance(v, k) for k in (list, tuple)] results on Python 2.X: results on Python 3.X: So some of the code is not working after migration to Python 3 =( |
|
Yes, that does appear to be a separate bug, and is obviously not tested. We could make use of |
|
jnothman@ yep, I can implement a tests for the usage of collections.Sequence. Probably will start with filing a bug =) PS: I updated the text of the Errors |
| 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)] | ||
| if True not in check: | ||
| raise ValueError("Parameter values should be a list.") | ||
| raise ValueError("Parameter ({0}) value should be a list." |
There was a problem hiding this comment.
I have a hard time parsing this sentence. Currently when searching over parameter alpha, it says something like
Parameter (alpha) value should be a list.
That seems not very intuitive. Each parameter value for alpha should be a float.
Maybe " Parameter values for parameter {} need to be a sequence". ?
There was a problem hiding this comment.
amueller@: your text look better, indeed. Thank you. PS: pull-request has been updated.
Old value: Parameter ({0}) value should be a list.
New value: Parameter values for parameter ({}) need to be a sequence.
|
jnothman@ anything else you want me to change in the request? |
|
Well, I know for one thing that |
…Error have been added.
… test_grid_search_param_grid_includes_sequence_of_a_zero_length.
|
jnothman@ thank you! I did't know about the {} and Python 2.6. Error text has been updated. Plus I have added two tests that are covering cases when ValueError are thrown. |
| ValueError, | ||
| "Parameter values for parameter (C) need to be a sequence.", | ||
| GridSearchCV, | ||
| None, |
There was a problem hiding this comment.
I think you should provide an estimator here. The test case is unnecessarily pathological.
| 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)] | ||
| if True not in check: | ||
| raise ValueError("Parameter values should be a list.") | ||
| raise ValueError("Parameter values for parameter ({0}) need to " |
There was a problem hiding this comment.
These should be strictly <80 characters
|
LGTM. We usually require a second reviewer before merging. |
|
LGTM as well. Merging, thanks a lot @b0noI! |
Reference Issue
N/A
What does this implement/fix? Explain your changes.
ValueError that is thrown by _check_param_grid method now includes parameters name when values of a parameter have incorrect type.
Old ValueError:
ValueError: Parameter values should be a list.
New ValueError:
ValueError: Parameter(key1) values should be a non-empty list.
Any other comments?
p.items() - creates a new list that includes tuples from the original dict on the Python2.x, however amount of a params should not be big, so it should not be a problem. On a Python 3.x items() crates iterator.
2 tests are broken:
sklearn.feature_extraction.tests.test_image.test_connect_regions ... FAIL
sklearn.feature_extraction.tests.test_image.test_connect_regions_with_grid ... FAIL
they have been broken before my commit. No new breakage is introduced by the commit itself.