ENH: Make sorting optional for cKDTree.query_ball_point() #8908
Conversation
There was a problem hiding this comment.
As noted in the linked PR, it seems your editor is automatically adjusting whitespace quite a lot; this made the diff a bit tedious to parse.
That said, the github diff setting that allows for ignoring whitespace changes is helpful to narrow down the pertinent changes here.
I suspect that magnitude of whitespace adjustment is a bit out of scope for this PR, but the actual content changes seem to be reasonably small & you've added some unit tests, which looks promising at first glance (haven't check this in detail yet).
Might be worth considering addition of an asv benchmark test for the performance of the no-sorting approach with a multi-point query?
JesseLivezey
force-pushed
the
optional_sort
branch
3 times, most recently
from
3872858
to
2c08cbb
Compare
|
@tylerjereddy removed all of the white-space changes. I also think I added an |
|
Seems to agree with @sturlamolden's opinion in gh-8838. Sturla, does this look fine to you? |
|
I haven't tested this code, but from the looks of it yes. @rgommers |
|
@sturlamolden @rgommers Anything I can do to push this along? Does scipy require a rebase on master to merge? |
You just did, pinging us. We have a bit of a review backlog; the list of open PRs never seems to grow shorter.
No, if the PR can be merged (which it can currently) then it's all good. |
|
@JesseLivezey If you could rebase / force push that would be helpful to flush through CI since it has been a few months for this PR. Apart from that, seems like I can merge before the version 1.2.x branching very soon. |
| Sorts returned indicies if True and does not sort them if False. If | ||
| None, does not sort single point queries, but does sort | ||
| multi-point queries which was the behavior before this option | ||
| was added. |
There was a problem hiding this comment.
This probably warrants a .. versionadded:: directive.
scipy/spatial/tests/test_kdtree.py
Outdated
| @@ -1330,3 +1330,28 @@ def test_short_knn(): | |||
| [0., 0.01, np.inf, np.inf], | |||
| [0., 0.01, np.inf, np.inf], | |||
| [0., np.inf, np.inf, np.inf]]) | |||
|
|
|||
| class Test_sorted_query_ball_point: | |||
There was a problem hiding this comment.
We still support python 2.7, where this is an old-style class. Please inherit from object explicitly.
JesseLivezey
force-pushed
the
optional_sort
branch
from
2c08cbb
to
d001670
Compare
|
@tylerjereddy I rebased and pushed and I think I addressed @ev-br's changes. |
tylerjereddy
changed the title
There was a problem hiding this comment.
Reviewer comments addressed and CI is all green so merging, thanks @JesseLivezey.
If you could add a concise note to the release notes, that would be helpful.
Previously,
cKDTree.query_ball_point()would not sort the returned indices for single point queries, but did sort them for multi-point queries.For multi-point queries with large numbers of returned indices, sorting slowed down
query_ball_point()significantly.The PR adds a new optional kwarg which allows the returned indices to be explicitly sorted or not. The default (
None) has the same inconsistent behavior as before.Closes #8838