Skip to content

[MRG + 1] FIX for LassoLarsCV on with readonly folds #4684

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
Merged
merged 1 commit into from
May 20, 2015
Merged

[MRG + 1] FIX for LassoLarsCV on with readonly folds #4684

merged 1 commit into from
May 20, 2015

Conversation

ogrisel
Copy link
Member

@ogrisel ogrisel commented May 7, 2015

FIX #4597

@ogrisel ogrisel added the Bug label May 7, 2015
@ogrisel ogrisel added this to the 0.16.2 milestone May 7, 2015
@amueller
Copy link
Member

amueller commented May 7, 2015

do all four arrays need to be writable?

@amueller
Copy link
Member

amueller commented May 7, 2015

The answer is yes. LGTM.

@amueller amueller changed the title [MRG] FIX for LassoLarsCV on with readonly folds [MRG + 1] FIX for LassoLarsCV on with readonly folds May 7, 2015
amueller
amueller reviewed May 7, 2015
View reviewed changes
sklearn/linear_model/tests/test_least_angle.py
@@ -428,6 +434,24 @@ def test_no_warning_for_zero_mse():
assert_true(np.any(np.isinf(lars.criterion_)))


def test_lars_path_readonly_data():
Copy link
Member

Choose a reason for hiding this comment

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

you could probably do a more light-weight test by using stringIO and np.memmap? I don't like IO in tests.... but seems good enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

mmap is a system call, you cannot do it on a Python StringIO instance.

@amueller
Copy link
Member

amueller commented May 7, 2015

It might be interesting to add a flag to joblib to force it to memmap in parallel for debugging and testing purposes. Maybe even to memmap with n_jobs=1?

@ogrisel
Copy link
Member Author

ogrisel commented May 20, 2015

It might be interesting to add a flag to joblib to force it to memmap in parallel for debugging and testing purposes. Maybe even to memmap with n_jobs=1?

Automatic memmaping is done internally by the MemmapingPool. The whole multiprocessing backend is skipped when n_jobs=1. I don't think we want to change that.

@ogrisel
Copy link
Member Author

ogrisel commented May 20, 2015

@agramfort I would appreciate a second review on this. I met today a real user that was impacted by this issue.

@agramfort
Copy link
Member

LGTM

does it fix the pb of this user?

@amueller
Copy link
Member

it should.

amueller added a commit that referenced this pull request May 20, 2015
@amueller
Merge pull request #4684 from ogrisel/readonly-lars-path
6873c9f 
[MRG + 1] FIX for LassoLarsCV on with readonly folds
@amueller amueller merged commit 6873c9f into scikit-learn:master May 20, 2015
@ogrisel ogrisel mentioned this pull request Jun 24, 2015
Merged
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
Bug
Projects
None yet
Milestone
0.16.2
Development

Successfully merging this pull request may close these issues.

LassoLarsCV error if n_jobs > 1
3 participants
@ogrisel @amueller @agramfort