Skip to content
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.

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

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

Closed

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
# 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 ?

@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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants