8000 WIP: Migrate to Cython memoryviews in sklearn.utils by rth · Pull Request #11964 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

WIP: Migrate to Cython memoryviews in sklearn.utils #11964

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 3 commits into from
Closed

WIP: Migrate to Cython memoryviews in sklearn.utils #11964

wants to merge 3 commits into from

Conversation

rth
Copy link
Member
@rth rth commented Sep 1, 2018 •
edited
Loading

This aims migrate sklearn.utils from the numpy buffer interface in cython to memoryviews.

Addresses part of #10624
Also added a tests make sure we are not in the case #10624 (comment)

WDYT @lesteve ?

@rth rth changed the title WIP: Migrate to memoryviews in sklearn.utils WIP: Migrate to Cython memoryviews in sklearn.utils Sep 1, 2018
@rth
Copy link
Member Author
rth commented Sep 1, 2018 •
edited
Loading

After investigating with @lesteve it turns out the tests were not failing because X.astype('float64') does make a copy for float64 (e.g. here), so the cython functions was not getting the read-only mmap.

Once that is fixed in #11966, this PR will not work unfortunately due to #10624 (comment) :/

@rth
Copy link
Member Author
rth commented Sep 2, 2018

Or more explicitly it would be necessary to fix cython/cython#1772 to make this possible

@lesteve lesteve mentioned this pull request Sep 2, 2018
Merged
42 tasks
@jakirkham
Copy link
Contributor

FWIW astype does take a copy keyword argument. So this copying behavior could be disabled. Though it will still copy if the type does not match.

@rth
Copy link
Member Author
rth commented Sep 26, 2018

FWIW astype does take a copy keyword argument. So this copying behavior could be disabled. Though it will still copy if the type does not match.

Yes, but than we get the overhead of a copy in the hope of gaining something when transitioning to memoryviews.

Generally the goal of this PR was to check whether we could easily use memoryviews. We cant (except for workarounds you mentioned) until the corresponding Cython issue is fixed. Closing this for now.

@rth rth closed this Sep 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rth @jakirkham
0