FIX sort radius neighbors results when sort_results=True and algorithm="brute" #18612
Conversation
| if not return_distance: | ||
| raise ValueError("return_distance must be True " | ||
| "if sort_results is True.") |
There was a problem hiding this comment.
Should this validation be done in all cases, and not just with 'brute'?
There was a problem hiding this comment.
This validation is already done in the method query_radius for "ball_tree" and "kd_tree", and it is not necessary with a precomputed sparse graph.
| raise ValueError("return_distance must be True " | ||
| "if sort_results is True.") | ||
| for ii in range(len(neigh_dist)): | ||
| order = np.argsort(neigh_dist[ii], kind='mergesort') |
There was a problem hiding this comment.
just curious, why do we need the sort to be stable here?
There was a problem hiding this comment.
I used mergesort to be consistent with the sorting done in _check_precomputed, but I don't remember why we chose it.
| if not return_distance: | ||
| raise ValueError("return_distance must be True " | ||
| "if sort_results is True.") | ||
| for ii in range(len(neigh_dist)): |
There was a problem hiding this comment.
Is there a way we can vectorize this operation instead of looping over all samples? IIUC the arrays in neigh_dist may be of different lengths so that might not be possible, but I'm not familiar enough to confirm
There was a problem hiding this comment.
It is not possible, as you said each row is an array of different length.
Co-authored-by: Nicolas Hug <[email protected]>
Co-authored-by: Nicolas Hug <[email protected]>
Co-authored-by: Nicolas Hug <[email protected]>
…m="brute" (scikit-learn#18612) Co-authored-by: Nicolas Hug <[email protected]>
…m="brute" (scikit-learn#18612) Co-authored-by: Nicolas Hug <[email protected]>
3 participants
Fixes #18583.
Calling
radius_neighborsmethod ofNearestNeighborswas not sorting the result by distance whensort_results=True, when fitted withalgorithm="brute".This PR fixes this issue, and clarifies the docstring.