Skip to content
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

FIX Fallback to ball_tree using minkowski with w for kd_tree #22241

Merged
merged 13 commits into from
Jan 22, 2022
Merged

FIX Fallback to ball_tree using minkowski with w for kd_tree #22241

merged 13 commits into from
Jan 22, 2022

Conversation

jjerphan
Copy link
Member

@jjerphan jjerphan commented Jan 18, 2022 •
edited
Loading

Reference Issues/PRs

This is follow-up for #21741.

What does this implement/fix? Explain your changes.

Add better test coverage for weighted minkowski when KDTree is used for kneighbors.
Fallback to BallTree.

@jjerphan jjerphan marked this pull request as ready for review January 19, 2022 08:59
@jjerphan jjerphan requested a review from ogrisel January 19, 2022 08:59
@jjerphan jjerphan mentioned this pull request Jan 19, 2022
Closed
ogrisel
ogrisel reviewed Jan 19, 2022
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR. I think Minkowski with non None w should rather fallback to ball_tree in low dim as explained below.

Other than that a few nitpicks and LGTM.

sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
sklearn/neighbors/tests/test_neighbors.py Outdated Show resolved Hide resolved
sklearn/neighbors/tests/test_neighbors.py Outdated Show resolved Hide resolved
sklearn/neighbors/tests/test_neighbors.py Outdated Show resolved Hide resolved
sklearn/neighbors/tests/test_neighbors.py Show resolved Hide resolved
@ogrisel
Copy link
Member

ogrisel commented Jan 19, 2022

Somewhat related to #21873 about making the scikit-learn implementation consistent with the new scipy parametrization (for discoverability when reviewing).

jjerphan and others added 2 commits January 19, 2022 14:49
@jjerphan @ogrisel
Address review comments
6f19231 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@jjerphan
Warn when falling back on BallTree
1feaf55
ogrisel
ogrisel approved these changes Jan 19, 2022
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, assuming CI is green with the latest change.

There is just a skip case from a previous version of the PR that seems no longer necessary:

sklearn/neighbors/tests/test_neighbors.py Outdated Show resolved Hide resolved
@ogrisel ogrisel changed the title MAINT Fallback to 'brute' when using 'wminkowski' for 'kdtree' Fix Fallback to 'ball' when using 'wminkowski' for 'kdtree' Jan 19, 2022
@ogrisel ogrisel changed the title Fix Fallback to 'ball' when using 'wminkowski' for 'kdtree' Fix Fallback to 'ball_tree' when using 'wminkowski' for 'kd_tree' Jan 19, 2022
@ogrisel ogrisel changed the title Fix Fallback to 'ball_tree' when using 'wminkowski' for 'kd_tree' Fix Fallback to 'ball_tree' when using 'minkowski' with a w params for 'kd_tree' Jan 19, 2022
@jjerphan @ogrisel
Remove unnecessary test
08b464e 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@jjerphan jjerphan requested review from thomasjpfan and removed request for ogrisel January 20, 2022 09:47
@ogrisel
Copy link
Member

ogrisel commented Jan 21, 2022

I wonder if this can help fix #22257 with the development versions of numpy/scipy.

@ogrisel
Copy link
Member

ogrisel commented Jan 21, 2022 •
edited
Loading

Let me try in a new conda env with the latest nightly buids of scipy:

 pip install --pre --extra-index https://pypi.anaconda.org/scipy-wheels-nightly/simple scipy numpy

@ogrisel
Copy link
Member

ogrisel commented Jan 21, 2022

I confirm this PR fixes #22257 ! 🥳🎉

@jjerphan
Copy link
Member Author

I confirm this PR fixes #22257 ! :partying_facetada:

« D'une pierre, deux coups. » 🤙

@glemaitre glemaitre changed the title Fix Fallback to 'ball_tree' when using 'minkowski' with a w params for 'kd_tree' FIX Fallback to 'ball_tree' when using 'minkowski' with a w params for 'kd_tree' Jan 21, 2022
lorentzenchr
lorentzenchr approved these changes Jan 22, 2022
View reviewed changes
Copy link
Member

@lorentzenchr lorentzenchr left a comment

Choose a reason for hiding this comment

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

LGTM with the usual nitpick:wink:

sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
sklearn/neighbors/tests/test_neighbors.py Show resolved Hide resolved
sklearn/neighbors/tests/test_neighbors.py Outdated Show resolved Hide resolved
sklearn/neighbors/tests/test_neighbors.py Outdated Show resolved Hide resolved
sklearn/neighbors/tests/test_neighbors.py Outdated Show resolved Hide resolved
thomasjpfan
thomasjpfan approved these changes Jan 22, 2022
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Small nit, otherwise LGTM

sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
jjerphan and others added 3 commits January 22, 2022 15:04
@jjerphan @lorentzenchr
Update sklearn/neighbors/_base.py
9df5672 
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
@jjerphan @lorentzenchr @thomasjpfan
Apply comments from reviews
b17f59c 
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@jjerphan
[scipy-dev] Testing coverage
a320e9c
@thomasjpfan thomasjpfan changed the title FIX Fallback to 'ball_tree' when using 'minkowski' with a w params for 'kd_tree' FIX Fallback to ball_tree using minkowski with w for kd_tree Jan 22, 2022
@thomasjpfan thomasjpfan merged commit 6145cae into scikit-learn:main Jan 22, 2022
@jjerphan jjerphan deleted the kdtree-wminkowski-fallback branch October 21, 2022 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@lorentzenchr lorentzenchr lorentzenchr approved these changes

Assignees
No one assigned
Labels
Build / CI module:neighbors No Changelog Needed Waiting for Reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jjerphan @ogrisel @thomasjpfan @lorentzenchr