Skip to content

MAINT Remove get_memview_* helpers in KDTree & BallTree #15097

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
Closed
wants to merge 4 commits into from
Closed

MAINT Remove get_memview_* helpers in KDTree & BallTree #15097

wants to merge 4 commits into from

Conversation

rth
Copy link
Member

@rth rth commented Sep 26, 2019

The get_memview_* Cython helpers in KDTree and BallTree were used because at the time numpy/Cython didn't fully support memoryviews and now can be removed.

This was adapted from larsmans's refactoring PR in #4217

rth
rth commented Sep 26, 2019
View reviewed changes
sklearn/neighbors/binary_tree.pxi
@@ -1322,7 +1258,7 @@ cdef class BinaryTree:

# flatten X, and save original shape information
np_Xarr = X.reshape((-1, self.data.shape[1]))
cdef DTYPE_t[:, ::1] Xarr = get_memview_DTYPE_2D(np_Xarr)
cdef const DTYPE_t[:, ::1] Xarr = np_Xarr
Copy link
Member Author

Choose a reason for hiding this comment

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

Input array can be read-only, in which case setting it to a non const memoryview would fail.

This was referenced Sep 27, 2019
Open
Merged
@adrinjalali
Copy link
Member

Do we get some unintended speed degradation here? Could you maybe make sure there are no odd python interactions after the change?

@rth rth mentioned this pull request Nov 2, 2019
Closed
@thomasjpfan thomasjpfan self-assigned this Nov 2, 2019
@alexhenrie alexhenrie mentioned this pull request Jan 17, 2020
Merged
@rth rth mentioned this pull request Feb 28, 2020
Open
@rth rth mentioned this pull request Apr 7, 2020
Closed
@rth
Copy link
Member Author

rth commented Apr 7, 2020

Do we get some unintended speed degradation here? Could you maybe make sure there are no odd python interactions after the change?

I did benchmarks at the time, that showed no significant performance cost, but can't find them anymore. I'm waiting for official ASV benchmarks to be added #16723 so we have an official reference point.

Base automatically changed from master to main January 22, 2021 10:51
@jjerphan
Copy link
Member

@rth : are you still interested in pursuing this PR? 🙂

@rth
Copy link
Member Author

rth commented Apr 14, 2021

@jjerphan Feel free to continue it in a separate PR and ping me for review :) I think the only thing remaining was to resolve conflicts and run some benchmarks to confirm.

@jjerphan
Copy link
Member

Thanks for the quick reply!
I am following up a forest of PRs for neighbours.BinaryTrees and this one looks relevant.

@jjerphan jjerphan mentioned this pull request Apr 14, 2021
Merged
@rth rth closed this in #19893 Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@thomasjpfan thomasjpfan

Labels
cython module:neighbors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rth @adrinjalali @jjerphan @thomasjpfan