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

TST Improve tests for neighbor models with X=None #30101

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 3 commits into from
Oct 21, 2024

Conversation

ogrisel
Copy link
Member

@ogrisel ogrisel commented Oct 18, 2024

While reviewing #30047 by @dkobak, I thought we could improve the tests as follows:

  • make them run faster by using smaller data;
  • explicitly test for all algorithms;
  • avoid raising 0 neighbor within radius warnings;
  • explain better the statistical meaning of the assertions.

Copy link

github-actions bot commented Oct 18, 2024

✔️ Linting Passed

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

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

@dkobak
Copy link
Contributor

dkobak commented Oct 18, 2024

Thanks @ogrisel, this looks really good. Apologies for not implementing these things myself, and thanks for improving my tests! I don't have much experience with setting up unit tests.

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

sklearn/neighbors/tests/test_neighbors.py Outdated Show resolved Hide resolved
@OmarManzoor OmarManzoor enabled auto-merge (squash) October 21, 2024 06:35
@ogrisel
Copy link
Member Author

ogrisel commented Oct 21, 2024

@dkobak no apologies needed. Thanks for the PR.

@OmarManzoor OmarManzoor merged commit 15d5a06 into scikit-learn:main Oct 21, 2024
28 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.

4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.