Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

ENH Make KNeighborsClassifier.predict handle X=None #30047

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

Merged
merged 8 commits into from
Oct 18, 2024

Conversation

dkobak
Copy link
Contributor

@dkobak dkobak commented Oct 11, 2024

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?

Copy link

github-actions bot commented Oct 11, 2024

✔️ Linting Passed

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

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

Copy link
Member

@adrinjalali adrinjalali left a comment

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
Copy link
Contributor

@OmarManzoor OmarManzoor left a comment

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

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

dkobak commented Oct 18, 2024

@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
Contributor

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

sklearn/neighbors/tests/test_neighbors.py Outdated Show resolved Hide resolved
@dkobak
Copy link
Contributor Author

dkobak commented Oct 18, 2024

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!

Copy link
Contributor

@OmarManzoor OmarManzoor left a comment

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
30 checks passed
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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.