Skip to content

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

Closed
rth wants to merge 3 commits intoscikit-learn:masterfrom
rth:_csr_row_norms-memoryviews
Closed

WIP: Migrate to Cython memoryviews in sklearn.utils#11964
rth wants to merge 3 commits intoscikit-learn:masterfrom
rth:_csr_row_norms-memoryviews

Conversation

@rth
Copy link
Copy Markdown
Member

@rth rth commented Sep 1, 2018

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
Copy Markdown
Member Author

rth commented Sep 1, 2018

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
Copy Markdown
Member Author

rth commented Sep 2, 2018

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

@jakirkham
Copy link
Copy Markdown
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
Copy Markdown
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants