8000 [MRG+1] Make sparse_encode give copy on write arrays by vighneshbirodkar · Pull Request #5998 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG+1] Make sparse_encode give copy on write arrays #5998

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

[MRG+1] Make sparse_encode give copy on write arrays #5998

wants to merge 3 commits into from

Conversation

vighneshbirodkar
Copy link
Contributor

Addresses #5956

@MechCoder MechCoder changed the title Make sparse_encode give copy on write arrays [MRG] Make sparse_encode give copy on write arrays Jan 15, 2016
@MechCoder
Copy link
Member

LGTM

@MechCoder MechCoder changed the title [MRG] Make sparse_encode give copy on write arrays [MRG+1] Make sparse_encode give copy on write arrays Jan 15, 2016
GaelVaroquaux
GaelVaroquaux reviewed Jan 17, 2016
View reviewed changes
sklearn/decomposition/tests/test_dict_learning.py Outdated
# Test that SparseCoder does not error by passing reading only
# arrays to child processes

init_dict = np.random.rand(500, 64)
Copy link
Member

Choose a reason for hiding this comment

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

You should not use the global random number generator in the test. Always create a local one using:

rng = np.random.RandomState(0)

and then draw samples from this random number generator.

@vighneshbirodkar
Copy link
Contributor Author

@GaelVaroquaux I have implemented your suggestion. Can you give this a review again ?

@vighneshbirodkar
Copy link
Contributor Author

This failed due to an HTTP failure. @MechCoder , can you re run this ?

@vighneshbirodkar
Copy link
Contributor Author

@GaelVaroquaux Could you give this another review ?

@vighneshbirodkar
Copy link
Contributor Author

I added come comments in the unit test to make it more clear

@ogrisel
Copy link
Member
ogrisel commented Apr 19, 2016

I am -1 on this change. mmap_mode='c' breaks under windows with large arrays:

http://stackoverflow.com/questions/24406937/scikit-learn-joblib-bug-multiprocessing-pool-self-value-out-of-range-for-i-fo

This is why I changed the default to mmap_mode='r' in joblib. The solution is to make our sparse_encode code accept to work on readonly buffers and do the copy manually only if needed. For instance you could reuse _check_copy_and_writeable from sklearn/linear_model/least_angle.py that was precisely introduced for similar purpose.

@vighneshbirodkar
Copy link
Contributor Author

@ogrisel How about now ?
< 8000 /div>

@jnothman jnothman added this to the 0.20 milestone Jun 18, 2017
@ogrisel
Copy link
Member
ogrisel commented Jun 22, 2018

Sorry for the slow feedback @vighneshbirodkar. It looks good to me!

@ogrisel
Copy link
Member
ogrisel commented Jun 22, 2018

I will do the conflict resolution and merge.

@ogrisel ogrisel mentioned this pull request Jun 22, 2018
Merged
@ogrisel
Copy link
Member
ogrisel commented Jun 22, 2018

Closing in favor of #11346.

@ogrisel ogrisel closed this Jun 22, 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
0.20
Development

Successfully merging this pull request may close these issues.
5 participants
@vighneshbirodkar @MechCoder @ogrisel @GaelVaroquaux @jnothman
0