[MRG + 1] Friendly error on KNeighbors(n_neighbors=0 or not convertible to int) #4312
Conversation
| @@ -258,6 +258,19 @@ def _fit(self, X): | |||
| else: | |||
| raise ValueError("algorithm = '%s' not recognized" | |||
| % self.algorithm) | |||
|
|
|||
| if self.n_neighbors is not None: | |||
There was a problem hiding this comment.
n_neighbors doesn't exist in all instances of NeighborsBase (i.e. where radius neighbors are intended).
I also don't understand why anyone would set n_neighbors to None. Or why we want to handle things like strings...
There was a problem hiding this comment.
It is set to None in _init_params, right? Don't you think this is the right fix?
There was a problem hiding this comment.
No informative error appears to be raised for n_neighbors == None for now, although the discussion seems to have converged to that. Perhaps:
if self.n_neighbors is None or self.n_neighbors <= 0:
raise ValueError(
...```There was a problem hiding this comment.
Some classes like radiusneighbors which inherits neighborbase do not need n_neighbors. n_neighbors could be None, I think. @banilo
There was a problem hiding this comment.
Indeed, none is valid in this part of the code.
There was a problem hiding this comment.
It would be possible to add a check in "predict", too, in a place where None is not valid, but I'm not sure that is necessary.
|
maybe we want to check |
|
I may have missed some of the discussion, but I've not known scikit-learn On 2 March 2015 at 11:01, Andreas Mueller notifications@github.com wrote:
|
|
hm we don't really need to. But we should check for zero, as that gives a weird error message downstream and can easily happen. Maybe just check |
|
@jnothman checked @amueller the code now just checks ```n_neighbors > 0 ''' if it is not None. |
|
Looks good to me, thanks. |
bb4704e to
d2cdcb5
Compare
d2cdcb5 to
2a5eba2
Compare
| nbrs = cls(n_neighbors=-1) | ||
| assert_raises(ValueError, | ||
| nbrs.fit, | ||
| X, y) |
There was a problem hiding this comment.
style: please use a single line for function calls that fit in 80 columns.
Related: you should run the pep8 linter to identify code style issues (only consider the errors that match the lines that you edit in your branch).
2a5eba2 to
9986aa5
Compare
|
Thanks @ogrisel ! Fixed style problem. BTW, the code above the line I added has the style problem as well, do I need to fix them? |
No let's keep this PR focused and limit possible merge conflicts. |
| @@ -830,6 +830,10 @@ def test_neighbors_badargs(): | |||
| assert_raises(ValueError, | |||
| nbrs.predict, | |||
| []) | |||
| if isinstance(cls, neighbors.KNeighborsClassifier) or \ | |||
| isinstance(cls, neighbors.KNeighborsRegressor): | |||
There was a problem hiding this comment.
again style: please use enclosing parens for the if condition instead of the \-escaping of the new line.
9986aa5 to
63812b7
Compare
|
Thanks!. I'd better to read PEP 8. When I use |
|
Then you have a very old version of the tool. |
Sure, it's just a recommendation for the consistency with the scikit-learn code base. |
| if (isinstance(cls, neighbors.KNeighborsClassifier) or | ||
| isinstance(cls, neighbors.KNeighborsRegressor)): | ||
| nbrs = cls(n_neighbors=-1) | ||
| assert_raises(ValueError, nbrs.fit, X, y) |
There was a problem hiding this comment.
Similarly to the comnent above, not both cases appear to be covered by this regression test:
a) n_neighbors <= 1
b) n_neighbors == None
There was a problem hiding this comment.
a) n_neighbors could be 1, right? @banilo
b) like above comment
|
I am merging this, as it is a net improvement over the code, although, as suggested, it could be improved even more. |
[MRG + 1] Friendly error on KNeighbors(n_neighbors=0 or not convertible to int)
This PR added some lines to
NeighborsBase._fitfor checkingn_neighbors. If it is 0 or not convertible to int, it raise ValueError. Fix #4296