Skip to content

Fix: RuntimeWarning by dividing by zero in test_radius_neighbors_classifier_zero_distance #19395

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Feb 12, 2021
Merged

Fix: RuntimeWarning by dividing by zero in test_radius_neighbors_classifier_zero_distance #19395

merged 5 commits into from
Feb 12, 2021

Conversation

nuka137
Copy link
Contributor

@nuka137 nuka137 commented Feb 8, 2021

Reference Issues/PRs

#19334

What does this implement/fix? Explain your changes.

The runtime error is caused by _weight_func function which will be passed to neighbors.RadiusNeighborsClassifier.
The dist value becomes zero in this test, and this makes the returned value inf.
So, I added small value 1e-8 to denominator to avoid small value division.

@jjerphan
Copy link
Member

jjerphan commented Feb 8, 2021

Hi again,

Similarly to suggestions in #19396, could you add a test to assert that the warning is not thrown?

@nuka137
Copy link
Contributor Author

nuka137 commented Feb 9, 2021

@jjerphan

Thanks for your review!
I added the no warning test.

jjerphan
jjerphan approved these changes Feb 9, 2021
View reviewed changes
Copy link
Member

@jjerphan jjerphan 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 @nuka137!

jeremiedbb
jeremiedbb reviewed Feb 9, 2021
View reviewed changes
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @nuka137, a few comments.

@nuka137
Copy link
Contributor Author

nuka137 commented Feb 9, 2021

Thanks @jeremiedbb , @glemaitre .
Your insight and direction about this issue are much better than mine.
I applied your requests.

jeremiedbb
jeremiedbb approved these changes Feb 10, 2021
View reviewed changes
Copy link
Member

@jeremiedbb jeremiedbb 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 @nuka137

ogrisel
ogrisel approved these changes Feb 12, 2021
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with this fix. Let's just document why we expect a warning here.

@ogrisel ogrisel merged commit 31b34b5 into scikit-learn:main Feb 12, 2021
@ogrisel
Copy link
Member

ogrisel commented Feb 12, 2021

Linter is green, merged! Thanks again @ nuka137 for your bearing with us :)

@nuka137 nuka137 deleted the fix_test_radius_neighbors_classifier_zero_distance branch February 13, 2021 00:34
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre left review comments

@ogrisel ogrisel ogrisel approved these changes

@jjerphan jjerphan jjerphan approved these changes

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees
No one assigned
Labels
module:neighbors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nuka137 @jjerphan @ogrisel @glemaitre @jeremiedbb