Skip to content

ENH Make KNeighborsClassifier.predict handle X=None - #30047

Merged
OmarManzoor merged 8 commits into
scikit-learn:mainfrom
dkobak:knn_loocv
Oct 18, 2024
Merged

ENH Make KNeighborsClassifier.predict handle X=None#30047
OmarManzoor merged 8 commits into
scikit-learn:mainfrom
dkobak:knn_loocv

Conversation

@dkobak

@dkobak dkobak commented Oct 11, 2024

Copy link
Copy Markdown
Contributor

Reference Issues/PRs

Fixes #29722 and #27747.

This makes predict(), predict_proba(), and score() of KNeighborsClassifier and RadiusNeighborsClassifier take X=None. In this case predictions for all training set points are returned, and points are NOT included into their own neighbors. This makes it consistent with the current behaviour of KNeighborsRegressor and RadiusNeighborsRegressor.

New functionality is described in docstrings (and added to the docstrings of KNeighborsRegressor and RadiusNeighborsRegressor).

Tests are added confirming that knn.fit(X, y).predict(None) is equivalent to cross_val_predict(knn, X, y, cv=LeaveOneOut()).

Sanity checking

@ogrisel asked me to add a notebook showing that this is a statistically sane thing to do (#29722 (comment)):

It would also be great to check in a side simulation study (in a notebook) to show that the variance of the LOO CV scheme is not significantly larger than the one obtained fors k-fold CV with k in {5, 10, 50} and for n_neighbors in {1, 10, 100} to confirm the outcome of the literature review in #27747 (comment). This side notebook should not part of the scikit-learn repo but linked in the PR to help assert that the new feature exposed in the PR is a methodologically sane thing to do.

I did this here: #29722 (comment). Is it sufficient to simply leave it there?

@github-actions

github-actions Bot commented Oct 11, 2024

Copy link
Copy Markdown

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 6fc430b. Link to the linter CI: here

@adrinjalali adrinjalali left a comment

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.

LGTM.

@dkobak dkobak changed the title Make KNeighborsClassifier.predict handle X=None ENH Make KNeighborsClassifier.predict handle X=None Oct 15, 2024

@OmarManzoor OmarManzoor left a comment

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.

Thanks for the PR @dkobak

Comment thread doc/whats_new/v1.6.rst Outdated
Comment thread sklearn/neighbors/tests/test_neighbors.py Outdated
dkobak and others added 2 commits October 18, 2024 12:43
Co-authored-by: Omar Salman <omar.salman2007@gmail.com>
@dkobak

dkobak commented Oct 18, 2024

Copy link
Copy Markdown
Contributor Author

@OmarManzoor After I commited your one-line change to v1.6.rst some checks started failing, and I don't understand why :-/

@OmarManzoor

Copy link
Copy Markdown
Contributor

It seems like there are some conflicts with main. Can you merge main in your branch and resolve the conflicts?

Comment thread sklearn/neighbors/tests/test_neighbors.py Outdated
@dkobak

dkobak commented Oct 18, 2024

Copy link
Copy Markdown
Contributor Author

Oh! Turns out that upcoming changes should not go into v1.6.rst anymore but into separate files in upcoming_changes folder. This was implemented only yesterday: #30081. I adapted my PR accordingly and now everything seems to pass. Thanks!

@OmarManzoor OmarManzoor left a comment

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.

LGTM. Thanks @dkobak

@OmarManzoor
OmarManzoor enabled auto-merge (squash) October 18, 2024 13:23
@OmarManzoor
OmarManzoor merged commit bcc6430 into scikit-learn:main Oct 18, 2024
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.

Make KNeighborsClassifier.predict and KNeighborsRegressor.predict react the same way to X=None

3 participants